Repository navigation
ci: derive the clippy package lists so every tested crate is linted (#2058) - #2098
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2098 +/- ##
==========================================
+ Coverage 80.34% 80.64% +0.30%
==========================================
Files 413 428 +15
Lines 196629 211453 +14824
Branches 196629 211453 +14824
==========================================
+ Hits 157979 170536 +12557
- Misses 33113 35172 +2059
- Partials 5537 5745 +208
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
From independent review of #2098. Both are comment/message accuracy; no behaviour change. - The comment above `Check the default features compile clean` claimed that `Run clippy on all offline crates` "cannot list them because they pull in ort-sys". That step now selects `cargo-args lint`, which does include engine and server, so the claim was false and contradicted the comment this PR adds twenty lines above it. The step is still worth keeping, for a different and now-stated reason: with resolver = "2" features unify across every package selected in one invocation, so the 55-package run can enable features on engine/server that neither crate enables on its own. - The lint-coverage failure hint said "the offline clippy steps select cargo-args lint"; the Windows ARM64 one selects offline-linux. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Independent review: REQUEST_CHANGES, both findings fixed in
|
From independent review of #2098. Both are comment/message accuracy; no behaviour change. - The comment above `Check the default features compile clean` claimed that `Run clippy on all offline crates` "cannot list them because they pull in ort-sys". That step now selects `cargo-args lint`, which does include engine and server, so the claim was false and contradicted the comment this PR adds twenty lines above it. The step is still worth keeping, for a different and now-stated reason: with resolver = "2" features unify across every package selected in one invocation, so the 55-package run can enable features on engine/server that neither crate enables on its own. - The lint-coverage failure hint said "the offline clippy steps select cargo-args lint"; the Windows ARM64 one selects offline-linux. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bbc0afe to
e93532a
Compare
|
|
Measured follow-up on the one risk I flagged in the PR body: the Windows ARM64 clippy lane. I wrote that this step gains 18 crates on a target I cannot build locally, and that I would read the lane before merging. That is still true, but I can now bound the exposure rather than just watch it, and the bound is tighter than I expected: the marginal compile surface this change adds to that lane is zero. Three measurements, all from this branch at 1. The step's set grows by 18 and loses nothing.
2. All 18 added crates are already compiled and unit-tested on
3. What that leaves. The residual exposure is not "will it compile" — that is already answered green on this target every run. It is lint diagnostics on already-compiling code that differ by target. The target-conditional surface across the 18 is small enough to read: 6 One caveat I am not going to paper over. "Already compiled" is a package-level claim. Net: I am no longer treating this lane as an unverifiable risk, but I am still going to read it before this merges, and if it goes red I will confirm the failure is in one of the 18 (not inherited) before touching anything. Commands are all metadata/text — no builds, no host lock taken. |
|
Job 97784617364, step 29 — Test step wrapper still refuses an empty filter (self-test): Reproduced locally, byte-identical message, before changing anything. Diagnosis. #2066's scanner enumerates every The contract was correct when written — every Fix ( Strictly stronger than the rule it replaces, in both directions — each cell mutation-tested:
That last row is why I added an anti-vacuity control on the classifier itself. Its passing verdict is "prints nothing" — which is also precisely what a block parser that reads nothing prints. The control asserts the classifier's own enumeration matches, substitution for substitution, what a flat
On my risk prediction. I flagged Everything else at |
|
Independent delta review of I am the author and cannot self-approve, so the fix commit went to an independent reviewer with instructions to attack six specific things and to verify by running, not by reading. Summary of what it tried, since that is the part that lets you judge the review's coverage rather than its conclusion:
One residual it reported, and I am recording rather than silently keeping. The classifier's Verdict quote: "Every defect I could produce is either loud (spurious red) or outside the threat model; the dangerous case this file exists to refuse — a bare token reaching a cargo command line — fails loud on every path I tried." Worktree left clean by the reviewer ( |
|
My fix worked: step 29 (Test step wrapper still refuses an empty filter) passed. Because it passed, the job reached step 30, which the previous run had skipped — steps run under Step 30, Not attributable to this PR, by measurement rather than assertion: this branch's entire diff is three files under Source: #2056, merged 12:03:16Z with Everything else at this head is green, including the lane I flagged as this PR's only real risk: Auto-merge stays armed on SQUASH and I am not touching it. It cannot fire while a required check is red, which is the correct outcome — no |
From independent review of #2098. Both are comment/message accuracy; no behaviour change. - The comment above `Check the default features compile clean` claimed that `Run clippy on all offline crates` "cannot list them because they pull in ort-sys". That step now selects `cargo-args lint`, which does include engine and server, so the claim was false and contradicted the comment this PR adds twenty lines above it. The step is still worth keeping, for a different and now-stated reason: with resolver = "2" features unify across every package selected in one invocation, so the 55-package run can enable features on engine/server that neither crate enables on its own. - The lint-coverage failure hint said "the offline clippy steps select cargo-args lint"; the Windows ARM64 one selects offline-linux. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d99c48c to
7b0c45f
Compare
|
Four separate things confirmed there, and I want to be explicit about why each is in the list rather than just quoting the tick:
The one that I want on the record, because it is the whole argument of #2058 in miniature: the second red on this PR was not mine. My step 29 passing is what let the job reach step 30 — these steps run under |
…2058) 21 workspace crates were compiled and tested by CI and linted by nothing. The test lanes select packages from a derived list (workspace_test_packages.py walks cargo metadata, so a new crate is tested by default); the clippy lanes selected from a hand-maintained -p list repeated verbatim in three places. Derived-vs-manual is a ratchet in one direction: every new crate joined the test set automatically and joined the lint set only if someone remembered, three times, in three lists. Nobody did, 21 times. The stated reason for the manual list was wrong. The instruction above it said to "confirm its normal+dev dependency tree contains no ort-sys/CUDA dependency" -- but onnx-runtime-ep-cpu and onnx-runtime-ep-api were already on that list and both pull onnx-genai-ort-sys, so the rule was already violated by the list it annotated. It does not matter either way: cargo clippy only ever *checks*. It never links and never runs a test binary, so the constraint that splits the test lanes into offline/ort-backed has no force for lint at all. - All three clippy -p lists (identical, 31 packages each) become `cargo-args lint`, a new lane meaning "every package some test lane compiles". Windows ARM64 keeps the offline set, which is still a superset of what it linted before. - `verify` gains a lint-coverage half: it fails if any tested package is reached by no clippy invocation in .github/workflows. Generator calls are expanded rather than skipped, so a computed -p list counts and a hand-written one cannot hide behind some other step computing one. - A self-test step runs both --simulate-missing and --simulate-unlinted and requires exit 1 *and* the matching message. --simulate-missing had existed since the guard was written and CI had never once invoked it. Measured on this branch: cargo clippy --locked --all-targets over all 55 packages exits 0, and all 55 appear as compiler artifacts in the lint graph, so the pass is not an empty selection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
From independent review of #2098. Both are comment/message accuracy; no behaviour change. - The comment above `Check the default features compile clean` claimed that `Run clippy on all offline crates` "cannot list them because they pull in ort-sys". That step now selects `cargo-args lint`, which does include engine and server, so the claim was false and contradicted the comment this PR adds twenty lines above it. The step is still worth keeping, for a different and now-stated reason: with resolver = "2" features unify across every package selected in one invocation, so the 55-package run can enable features on engine/server that neither crate enables on its own. - The lint-coverage failure hint said "the offline clippy steps select cargo-args lint"; the Windows ARM64 one selects offline-linux. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ot by name Rust quality went red on this PR at 'Test step wrapper still refuses an empty filter (self-test)'. The failure is real and it is mine: #2066's scanner enumerates every $(python ...) substitution in ci.yml and executes it to prove the expansion is options only, and this PR added a substitution that is not a generator -- the coverage self-test calls the script's 'verify' subcommand with a loop variable, which is unexpandable outside the step, so the cell reported 'did not run' and failed a tree that is fine. The contract was right when it was written (every substitution was a generator) and is now too broad (the script also answers 'verify', a checker whose output is prose and whose exit status is the signal). But which one a substitution is cannot be read off its name: what makes a bare word dangerous is that the expansion lands in a cargo command line. So classify by position -- inside a cargo step it must be 'cargo-args', outside one it must be a known checker -- and keep executing exactly the generators. That is strictly stronger than the rule it replaces, in both directions: - a generator emitting a bare token is still caught (mutation: making cargo-args append 'kernels::sneaky::' fails the cell on all five substitutions) - a checker spliced into a cargo command line is now caught, which the old name-blind execution would have reported only by accident - a new subcommand outside a cargo step is refused rather than ignored, so the next generator cannot arrive unexamined Added an anti-vacuity control on the classifier itself, because its passing verdict is 'prints nothing' -- which is also what a block parser that reads nothing prints. Neutering the parser leaves the verdict cell green and only the control red, which is the whole reason it is there. EXPECTED 33 -> 36. shellcheck clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…commands The classifier accepted any substitution containing "verify" outside a cargo step. An independent review called that a quiet pass with no reachable harm, and I deferred it. Main then added `self-test` and `verify-required-tier`, so the checker set is no longer one name and the unanchored match now has something to be sloppy about. Both arms now name the script and match the subcommand at a word boundary. Mutation-checked: a step running `... .py verifyish` outside a cargo command is refused by the anchored form and was accepted by the old one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7b0c45f to
c6b0193
Compare
An independent review of the rebase found two ways a package could be
credited with lint coverage it does not have. Both fail quietly -- the
package looks linted, the gate stays green, and nothing says otherwise --
which is the failure direction this gate exists to remove.
* a `-p` in a comment indented *deeper* than the clippy invocation was
appended to the block and harvested. The docstring claimed comments
could not grant coverage; it stopped a comment from *starting* a
block, not from contributing tokens inside one.
* a second cargo command in the same step was read as part of the
clippy invocation, so `cargo clippy -p a && cargo test -p b`, or the
two on separate lines of a `run: |`, credited `b` to clippy.
The truncation is applied to the joined block text rather than line by
line, because the line-wise form missed the `&&` shape -- caught by an
arm that failed when I wrote it that way.
`clippy_blocks` is split out of `clippy_commands` so the four
terminators can be driven from fixtures, and six arms now cover them.
Each was mutation-checked, and one had to be added after the fact: my
first "the next step is not credited" arm stayed green with the indent
terminator disabled, because the cargo truncation was catching that
input instead. An arm that passes for a reason other than the one it
names is not a control. Removing each terminator now kills only the arms
that name it: comment skip 1, cargo truncation 2, indent 1, blank line 1.
self-test 44/44; verify unchanged at 55 tested, all linted by 10
invocations.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Rebased onto The rebase was not mechanical — main built the same thing better
So I deleted my step. The Same control, one mechanism instead of two. The other conflict was additive ( Independent review: APPROVE, with two latent findings I chose not to deferI am the author, so an Opus It also found two quiet false-green vectors, both rated Low and not reachable on today's
The second one bit me while fixing it: I wrote the truncation line-wise, which handles The arm that passed for the wrong reason
Two of those arms exist only because the first matrix came back wrong. My "the next step's packages are not credited" arm stayed green with the indent terminator disabled — the cargo truncation was catching that input instead. And the blank-line branch was killed by nothing at all, on any input, including real
Also: the
|
#2098 derived the clippy package lists from the same resolver the test lanes use, closing #2058 -- 21 crates were compiled and tested by CI and linted by nothing. It spelled them as bare `cargo clippy $(...)`, which is the form this PR exists to remove: cargo clippy --locked --all-targets $(python ... cargo-args lint) -- -D warnings If the resolver fails or the lane name is wrong, the substitution is empty, the outer command still exits 0, and clippy lints the default members instead of the lane. That is #2058 re-opened, silently, in a lane whose entire purpose is to prove it stayed closed -- and two of the three sites are in required lanes. All three now bind the list and refuse an empty one, matching the six `cargo test` sites. Argv on the healthy path is byte-identical: 55 crates for `lint`, 49 for the ARM64 `offline-linux` list. A bad lane exits 2 with clippy never invoked. Binding the generator moves the lane name off the `cargo clippy` line, so `linted_packages` stopped seeing it -- the same trap `verify-required-tier` set earlier in this PR, and it fails in the dangerous direction: the package looks unlinted, `verify` goes red, and the obvious repair is to loosen the scanner. `clippy_blocks` now reads backwards for the assignment, bounded by indentation so it cannot leave the step, and credits it only when the invocation actually references the variable. Crediting every assignment in scope would be a join, not a dataflow read, and over-reading is the whole documented risk there. Both directions are falsified by name: permissive (credit every preceding assignment) -> `linted_packages refuses a lane the clippy call never references` FAILS. `verify` still exits 0 -- the over-read is invisible to production, so this arm is the only thing holding that edge. no backwards read -> `linted_packages credits a lane the clippy call reads from a variable` FAILS and `verify` exits 1. verify 55 tested / all linted by 10 invocations, verify-required-tier exit 0, self-test 48/48, test_step_test.sh 36/36, 9 guards and 0 unbound substitutions, job names byte-identical to main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…readers (#2142) (#2147) Closes #2142. ## The defect `COUNTERS_OBSERVER_CHILD_ENV` (`task_runtime/mod.rs`) is declared **ungated** while both of its readers are `#[cfg(target_os = "linux")]`. Off-Linux the constant is dead code, and those lanes build with `-D warnings`, so it is a hard build failure: ``` error: constant `COUNTERS_OBSERVER_CHILD_ENV` is never used --> crates\onnx-runtime-ep-cpu\src\task_runtime\mod.rs:932:11 = note: `-D dead-code` implied by `-D warnings` error: could not compile `onnx-runtime-ep-cpu` (lib test) due to 1 previous error ``` Introduced by #2125 (`85565fc5b`). The fix gives the constant the same cfg predicate as its two readers, so all three appear and disappear together. ## How I found it, and why it is not the PR that surfaced it It reddened `Rust (Windows ARM64)` on my #2098. The timing discriminates cleanly — that lane on **the same PR branch** was green twice before #2125 merged and red after, with no Rust in the diff at any point: | lane run | started | vs #2125 (merged 17:13:15Z) | result | |---|---|---|---| | #2098 @ `e93532ae0` | 11:42:56Z | before | **success** | | #2098 @ `d99c48c13` | 13:11:41Z | before | **success** | | #2098 @ `45530133f` | 19:17:16Z | after | **failure** | CI builds the *merge result*, so a PR lane can be red for a defect that is entirely `main`'s. The colour moved because `main` moved. ## Verification I could not check the real target locally — `cargo check --target aarch64-pc-windows-msvc` dies in `onnx-genai-ort-sys`'s bindgen step (`fatal error: 'stdlib.h' file not found`), needing a Windows SDK. **That failure says nothing about this change**, and I am recording it rather than quietly reporting the exit code, because a cross-target check that fails for toolchain reasons is the mirror image of the trap @Gaff pinned on `check_cross_compile.sh`: one direction false-passes without a toolchain, the other false-fails. So I proved the mechanism natively instead, by making the *readers* off-target on Linux — which is exactly the shape Windows sees — and varying only the constant's gate: | arm | const | readers | rc | `is never used` | expected | |---|---|---|---|---|---| | **A** pre-fix state | ungated | absent | 101 | **yes** | yes ✓ | | **B** with this fix | gated | absent | 0 | no | no ✓ | | **C** real tree on Linux | gated | present | 0 | no | no ✓ | Arm A reproduces CI's exact error text, so B is not a pass by compiling nothing — the control is non-vacuous. Arm C shows the Linux behaviour is unchanged: the test and its child still compile and are still gated exactly as before. **No test is disabled by this change**; the constant is simply present on precisely the targets that read it. Required-lane commands, run as spelled: ``` cargo fmt --all --check -> 0 cargo clippy -p onnx-runtime-ep-cpu --all-targets --locked -- -D warnings -> 0 ``` (Read via `${PIPESTATUS[0]}`, not the pipeline's status.) ## The part worth keeping Both affected lanes are **advisory**. The required set is `Fast (Linux x86_64)` + `Rust quality`, and both are Linux — so **a Linux-only cfg mistake is structurally invisible to the gate that guards merges**. #2125 merged green and was genuinely green on everything required. That is the same tier gap as #1915 (Miri red, not required), and it is a different problem from a required check being red and merged anyway. Recorded on the audit ledger in #2056 as such rather than as a bypass. *No admin bypass; normal auto-merge, waiting on required CI.* Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…it (#2121) Five `cargo test` steps in `ci.yml` — including the one in the **required** `Fast (Linux x86_64)` lane — derived their package set from an unguarded command substitution: ```yaml cargo test --locked $(python .github/scripts/workspace_test_packages.py cargo-args offline-linux) ``` If that substitution ever produces nothing, `cargo test` still runs — as a **bare** `cargo test` — and the step exits 0. ## Measured shell semantics Under GitHub's default step shell (`bash -e {0}`), not inferred: | form | resolver fails | step exit | cargo runs | |---|---|---|---| | `cargo test $(resolver …)` | exit 2, empty stdout | **0** | **yes, bare** | | `cargo test $(resolver …)` | exit 0, empty stdout | **0** | **yes, bare** | | `packages="$(resolver …)"` | exit 2 | 2 (`set -e` aborts) | no | | `packages="$(resolver …)"; test -n "$packages"` | exit 0, empty | 1 | no | Assignment is the load-bearing half: `set -e` **does** abort on a failing substitution in an assignment and **does not** abort on one inside an argument list. The `test -n` guard covers the remaining case where the resolver succeeds but prints nothing. ## What the degraded run actually tests A bare `cargo test` falls back to `default-members` (46 crates), not the lane set (49): - **14 crates silently dropped**, including the entire plugin/ABI/FFI surface. - **`onnx-genai-ort-sys` pulled in** — explicitly denylisted because its build script downloads a native ONNX Runtime — plus both PyO3 crates. **Severity.** The harm is *invariant in the outcome colour*: a **required** check silently stops exercising the 14-crate plugin/ABI surface. Whether the degraded run then goes red is contingent on network and environment — the denylisted `ort-sys` crate has a strong offline failure vector, so a **red lane with a misattributed cause** is likelier than a silent green — but what the lane *covers* is not contingent on anything. The defect is that the lane stops testing what it claims to test while its exit code stays authoritative, in either colour. (Framing corrected by the independent review; I had originally anchored on the red/green coin-flip, which is the symptom, not the defect.) ## The fix Each of the five sites becomes: ```yaml packages="$(python .github/scripts/workspace_test_packages.py cargo-args <lane>)" test -n "$packages" || { echo "::error::lane '<lane>' resolved to an empty package set"; exit 1; } cargo test --locked $packages ``` Sites: `fast-linux` (`offline-linux`, **required**), `rust-coverage` ×2, `rust-windows-arm64`, `cli-ort`. A rationale comment is added at the required site only. ## Mutation battery — 6 arms, effective Each arm names what it expects and asserts whether the *test command* was reached, discriminated by argv: | arm | exit | test cmd ran | packages | |---|---|---|---| | new_healthy | 0 | yes | 49 | | new_badlane | 2 | **no** | 0 | | new_emptyres | 1 | **no** | 0 | | old_healthy | 0 | yes | 49 | | old_badlane | **0** | **yes** | **0** ← defect | | old_emptyres | **0** | **yes** | **0** ← defect | Both defect arms fail on the old form and pass on the new one, so the guard is what changes the verdict. **Instrument bug found and fixed mid-battery, disclosed because it invalidated a first run:** my stub `cargo` also shadowed the resolver's *own* `cargo metadata` call, crashing the resolver and causing the recorder to log the resolver's invocation instead of the lane's. Every arm was contaminated — `new_healthy` reported a nonsensical `exit=1 pkgs=0`. Fixed by passing `metadata` through to the real cargo and discriminating recorded commands by argv. Same species as the errors this repo's measurement-discipline skill catalogues: the instrument resolved against itself instead of against the subject. ## Per-site verification All five shipped steps executed with a stub recorder to confirm the real argv: | site | lane | packages | exit | |---|---|---|---| | fast-linux (required) | offline-linux | 49 | 0 | | rust-coverage | offline-cross-platform | 48 | 0 | | rust-coverage (linux-only) | linux-only | 1 | 0 | | rust-windows-arm64 | offline-cross-platform | 48 | 0 | | cli-ort | ort-backed | 6 | 0 | `cli-ort`'s trailing `-- --test-threads=1` is preserved. YAML parses; 9 jobs; the `Fast (Linux x86_64)` and `Rust quality` check names are unchanged, so the ruleset's required checks still resolve. ## Related blind spot (not fixed here) `workspace_test_packages.py verify`, which runs in the required `rust-quality` lane, would catch a *crashed* resolver — but it **takes no lane argument and never reads `ci.yml`**, so a lane-name mismatch between the workflow and the script stays invisible while `verify` prints `coverage ok`. It also treats advisory and required lanes as equivalent evidence: it proves a crate is a member of *some* lane, not that any *enforced* lane runs it. That is the same cardinality-vs-identity trap as §9 of the measurement-discipline skill, and it is why the "no required lane runs these six crates' tests" gap survived. Left for a separate change, since making `verify` strict today would red `main` and the ruleset half is Justin's call. Base merged with current `main` (`0be2d23fe`) and re-validated before arming. Working as Holden (Security Engineer). --- ## Correction: the first push broke the three Windows legs The guard as first written was bash-only, and **four of the five sites also run on Windows**, where the default step shell is **pwsh**. CI failed on exactly `Rust coverage (Windows x86_64)`, `Rust (Windows ARM64)` and `CLI ORT (Windows x86_64)` — no other lane. A failure set that is exactly the Windows legs and nothing else is the signature of a shell-portability defect, and that is what it was. **Root cause, reproduced directly under pwsh** with `$ErrorActionPreference = 'stop'` and a *native* fake cargo: | form | shell | exit | test cmd | argv | |---|---|---|---|---| | inline `$( )` | pwsh | 0 | ran | `argc=14` — correct 6-crate set | | guarded assignment | pwsh | 1 | **never ran** | — | | guarded assignment | `bash --noprofile --norc -eo pipefail` | 0 | ran | 98 tokens — correct 49-crate set | **Why the original was portable and I mistook that for it being bash.** The resolver prints **one token per line**. bash word-splits that; pwsh returns it as an array and splats it element-wise into a *native* command. So `cargo test --locked $(resolver …)` happened to deliver the right argv under both shells. That portability was an accident of the output format, never a contract — and it is precisely what made the inline form look shell-agnostic while its failure mode was not. **Fix:** `shell: bash` on all five steps. `shell: bash` already appears in every affected job — including `rust-windows-arm64` itself, for "Install the pinned Rust toolchain" and "Resolve cargo cache target" — which is in-repo evidence that bash is present on `windows-11-arm`, rather than an assumption about the runner image. ### Instrument note My first pwsh reproduction used a **pwsh function** as the fake cargo and reported `argc=3`. That number is wrong for a 6-crate lane, and the discrepancy is the tell: pwsh passes an array to a *function* as a single object and splats it only for *native* commands. Re-running against a real executable gave `argc=14`. Had I not checked the number against what the lane should contain, I would have reported that pwsh mangles the argument list — a real-sounding conclusion about the wrong subject. That is the same failure as the `cargo metadata` contamination disclosed above, and the same one as the original defect: **the instrument resolved against itself instead of against the subject.** I measured `bash -e {0}` semantics, concluded "this is how the step behaves", and shipped it to four steps that were not running bash. The measurement was correct; its scope was not. A limitation is scoped to the thing that carries it. --- ## Second correction: CRLF, found only once the Windows lanes ran bash `shell: bash` landed correctly — the ARM64 log confirms `C:\Program Files\Git\bin\bash.EXE --noprofile --norc -e -o pipefail {0}` — and then the step failed on something the pwsh path had been hiding: ``` error: unexpected argument 'onnx-genai-cuda-version-guard^M ' found ``` **Windows Python writes stdout in text mode and translates `\n` into `\r\n`.** bash splits command substitution on `IFS`, which contains space, tab and newline but **not** carriage return — so every token kept a trailing `\r` and cargo saw it as part of the package name. PowerShell strips CRLF when it splits native output into an array, so this was invisible for exactly as long as the Windows lanes ran pwsh. So the inline form was portable for **two** independent accidents, not one: one-token-per-line output (which `package_args` documents), *and* pwsh's CRLF stripping (which nothing documented, because nothing depended on it). **Fixed at the producer**, not per-step, so a future call site cannot reintroduce it: the resolver now forces LF separators for the `cargo-args` path. Verified by emulating Windows text-mode stdout on Linux — a falsifier that fails without the change and passes with it: | producer | resolver bytes | bash tokens | argc | |---|---|---|---| | without fix | `-p\r\nmlas-sys\r\n` | `-p\r`, `mlas-sys\r` | 4 | | with fix | `-p\nmlas-sys\n` | `-p`, `mlas-sys` | 4 | **`argc` is 4 either way.** An argument-count assertion — the obvious way to check "did the right package set reach cargo" — passes under the defect. Only token *identity* separates them. That is the same cardinality-vs-identity trap as the `verify` blind spot described above, arriving a third time in the same change: `verify` counts crates and cannot see which lane; my first pwsh model counted `argc=3` and could not see it was measuring a function instead of a native command; and here the count is invariant across the bug. `verify` still reports `coverage ok: 55 tested, 5 denied`, and both lanes still resolve to 98 and 12 tokens. --- ## Rebased onto #2023, which changed what this PR has to satisfy `main` landed **#2023** ("run the ORT-backed crates' tests in a required lane") while this was in review. It closes the six-crate gap described above and adds two subcommands — `self-test` and `verify-required-tier`. Both conflicts were co-located edits where each side is wanted, and there is a **sixth** call site now, in `Rust quality`, which makes it the most load-bearing of the set. It is converted too. ### `verify-required-tier` refused the guarded form — correctly Its `packages_tested_by` credits a lane only when the `cargo-args` call and the `cargo test` occupy the **same fragment**. The guard puts them on separate lines, so nothing was attributed and the gate reported that no required check runs any crate's tests — all 55. That is the gate behaving as designed, not a bug in it. Its docstring anticipates exactly this class: > joining continuations moves attribution in the permissive direction, and this gate's whole thesis is that an over-read is fatal while an under-read merely fails loudly … the fix is to put the invocation on one line. Its prescribed remedy is precisely the thing this PR exists to undo, so the gate is **extended** rather than worked around — and extended in the direction its own thesis allows. ### The extension is a dataflow read, not a join A lane resolved into a shell variable is credited to a later `cargo test` **only if that command references the variable**. The reference is what does the crediting. Reassignment *replaces* a variable's lanes rather than adding to them, so shadowing the name with a non-lane value drops the credit rather than keeping it. Two self-test arms hold it there, and both are mutation-effective: | mutation | arm that fails | |---|---| | credit every assignment regardless of reference (**permissive**) | `packages_tested_by refuses a lane the cargo call never uses` | | remove the variable read (back to pre-extension) | `packages_tested_by credits a lane the cargo call uses` + `verify-required-tier (control)` | The permissive mutation is the one that matters: it is the failure mode the gate's author warned about, and the arm exists specifically to catch it rather than to confirm the happy path. ### Post-merge state ``` verify coverage ok: 55 tested, 5 denied verify-required-tier exit 0 -- 55 package(s) tested by ['Fast (Linux x86_64)', 'Rust quality'] windows ORT coverage ok: 6 package(s) via ['CLI ORT (...)'] self-test 37/37 arms behaved as stated (35 before, +2 mine) ``` Six sites guarded, zero inline substitutions remain, and the three-dot diff touches exactly two files. --- ## Fourth correction: the guard tripped a *different* gate, and that gate was right to be suspicious `Rust quality` — a **required** lane — went red on: ``` FAIL no cargo test step in ci.yml carries an unguarded name filter ``` The gate is `scripts/test_step_test.sh`, and it guards the same bug class this PR does: a bare word after `cargo test` is a **name filter**, and libtest reports `ok` / exit 0 for a filter that matches nothing. So an accidental positional silently converts "run the suite" into "run nothing, successfully" — the identical fail-open shape, one layer down. It flagged my steps because its scanner joins every line of a `run:` block with a space and reads the result as one argv. That premise held while every `cargo test` step in this file was a single line. **My guard makes three of them scripts**, so the guard's own words — `echo`, `::error::...`, `exit`, `1` — landed in the same argv as `cargo test` and were read as selectors. Worth being clear about the direction of the error: the gate produced a **false positive on a correct step**, not a false negative on a broken one. For a ban that is the right way to fail, and it is the same argument @Gaff made against word-boundarying `shape_dispatch_gate` — do not loosen a ban to accommodate a benign case if loosening it could also admit a real one. So I did not add an exemption for my steps. ### What I changed instead — the premise, in two places **1. Statement boundaries survive the accumulator.** The scanner discarded newlines, which is exactly the information needed to tell a guard *beside* the cargo call from arguments *to* it. Which style a block uses decides it: `run: |` keeps newlines as separators, `run: >-` folds them into spaces. The accumulator now records the block scalar style, splits on `;`, `&&`, `||` and real newlines, and scans only the fragment containing `cargo test`. **2. A variable is blanked only when it is a generator.** `$(...)` was already blanked as opaque, with the existing dynamic cell closing that hole by *executing* every generator and proving it emits options only. My form moves the substitution into a variable, so the variable needs the same treatment — but only when it was assigned from `$(...)`. `packages="some::filter::"` followed by `cargo test $packages` is still caught. That second point is where a widened ban usually acquires a quiet hole, so it is where I put the falsifiers. ### Falsifiers — each names the assertion it requires to be red Three steps added to the detector's own mutant fixture: a guarded step that must **not** be flagged, a guarded step with a trailing `kernels::something::` that **must** be, and `packages="kernels::something::"` (assignment that is *not* a generator) that must be. | mutation | want | got | assertion that failed | |---|---|---|---| | control | PASS | PASS | — | | record vars from **any** assignment, not only `$(...)` | FAIL | FAIL | `MUTATION: the detector finds an unguarded filtered step` | | read a literal block as folded (drop style detection) | FAIL | FAIL | same, + the real-`ci.yml` cell | | drop the statement split | FAIL | FAIL | same, + the real-`ci.yml` cell | | stop blanking generator vars (over-strict) | FAIL | FAIL | same, + the real-`ci.yml` cell | An earlier revision of the fixture passed all four mutations **for the wrong reason** — a leftover string literal from the blanked assignment happened to remain in the same fragment, so the arm went red on debris rather than on the filter. Only the real-`ci.yml` cell was actually carrying the claim. That is @Gaff's finding from #1995 arriving in my own battery: *a red-expecting check must name what it expects to be red*, and I would add — it must be red for the stated reason, which you only learn by reading which assertion failed rather than the exit code. Fixing the accumulator fixed the fixture too. `33/33` assertions pass. `shellcheck` is clean apart from the `SC1091` already present on `main` (2 occurrences on both sides). ## Blocked on a break that is not mine — #2142 `Rust (Windows ARM64)` and `Rust coverage (macOS arm64)` are red on `main` independently of this PR: `COUNTERS_OBSERVER_CHILD_ENV` is declared ungated but consumed only under `#[cfg(target_os = "linux")]`, so it is dead code off-Linux and these lanes build with `-D warnings`. From #2125 (`85565fc5b`). Filed as **#2142** with a two-arm reproduction; not fixing it here, as it is @sebastian's area and unrelated to this diff. Neither lane is required, but I am not merging while they are red. Worth naming why it survived: the required set is exactly `Fast (Linux x86_64)` + `Rust quality` and **both are Linux**, so a Linux-only `cfg` mistake is structurally invisible to required CI — the same tier gap @Gaff documented, reached from a different direction. --- ## Merged `main` again, and it had added three more of these `main` moved 13 commits, one of them **#2098** ("derive the clippy package lists so every tested crate is linted", closing #2058). It touches all three of my files, and git merged it without a conflict — which is worth saying out loud, because a clean textual merge is not a correct one. It added three new invocations in exactly the form this PR exists to remove: ```yaml - name: Clippy Linux offline crates # fast-linux (REQUIRED) run: >- cargo clippy --locked --all-targets $(python .github/scripts/workspace_test_packages.py cargo-args lint) -- -D warnings ``` plus `Run clippy on all offline crates` (`rust-quality`, **required**) and `Clippy Windows ARM64 offline crates`. The consequence is sharper here than for the test lanes. #2058 was *"21 crates are compiled and tested by CI and linted by nothing"*. If this substitution degrades, clippy lints the default members instead of the lane and exits 0 — so #2058 is silently re-opened **in the lane whose only purpose is to prove it stayed closed**. Two of the three are required. All three are now bound and guarded, matching the six `cargo test` sites. Healthy-path argv is byte-identical: | site | old vs new | argc | crates | |---|---|---|---| | `lint` (×2 sites) | **identical** | 116 | 55 | | ARM64 `offline-linux` | **identical** | 106 | 49 | | bad lane | — | exit **2**, clippy never invoked | — | `ci.yml` is now **9 guards, 0 unbound substitutions**, and the 9 job `name:` values are byte-identical to `main`. ### The same trap, third time, and it fails in the dangerous direction Binding the generator moves the lane name off the `cargo clippy` line, so `linted_packages` stopped seeing it — exactly what `verify-required-tier` did to me earlier in this PR. But the direction differs and that matters: a package then looks **unlinted**, `verify` goes red, and the obvious repair is to loosen the scanner. That would be the wrong repair for the same reason @Gaff gave for `shape_dispatch_gate`. `clippy_blocks` now reads backwards for the assignment, bounded by indentation so it cannot walk out of the step, and credits it **only when the invocation actually references the variable**. Crediting every assignment in scope would be a join rather than a dataflow read, and that scanner's docstring names over-reading as its whole risk. | mutation | arm that fails, by name | `verify` | |---|---|---| | credit every preceding assignment (**permissive**) | `linted_packages refuses a lane the clippy call never references` | **exit 0** | | remove the backwards read | `linted_packages credits a lane the clippy call reads from a variable` + `verify (control)` | exit 1 | The permissive row is the load-bearing one, and it makes the case for these arms better than I could: **the over-read is invisible to `verify`.** Production stays green, every package still looks linted, and nothing anywhere says otherwise. The new arm is the only thing standing on that edge. ### Local state ``` verify 55 tested, all linted by 10 clippy invocation(s) verify-required-tier exit 0 self-test 48/48 arms behaved as stated test_step_test.sh 36/36 assertions passed shellcheck clean apart from the SC1091 already on main ci.yml 9 guards / 0 unbound substitutions / job names identical to main ``` ## Advisory lanes red on `main`, both filed, neither mine | lane | cause | issue | |---|---|---| | `Rust (Windows ARM64)`, `Rust coverage (macOS arm64)` | `COUNTERS_OBSERVER_CHILD_ENV` ungated but consumed only under `cfg(linux)` — dead code off-Linux, `-D warnings` | **#2142** | | `CUDA compile (Linux x86_64)` | `capture_sync_contract`: NMS (#2130) and Unique (#2113) added unguarded `.synchronize()` without the reviewed allowlist entry | **#2149** | For the CUDA one the attribution is exact rather than argued: **#2130 is not in my branch at all** — it reaches CI only through the merge ref — and I reproduce the failure locally from the Unique half alone. My diff contains zero Rust files. Three advisory lanes red on `main` from three unrelated commits in one day is itself the finding. Required is `Fast (Linux x86_64)` + `Rust quality`, and **both are Linux**, so a Linux-only `cfg` mistake is not merely under-covered — it is structurally invisible to the required set, in every crate. --- ## Second independent review, and the six findings it returned The diff grew after the first review (three clippy sites from #2098, a second scanner repair), so I asked for a fresh independent Opus review of the whole thing rather than of the delta. Verdict **APPROVE**, with six non-blocking findings. All six are now fixed, and each is falsified by a named mutation arm rather than by inspection. | # | finding | repair | arm | |---|---|---|---| | a | `_generator_vars_before` could walk past a step boundary and credit the *previous* step's deeper-indented body | return `{}` when the invocation line is itself a list item or carries `run:` | `B1` | | b | `$packages` matched `$packages_extra` — a prefix collision granting credit to the wrong variable | match variable references whole | `E1` | | c | a plain assignment shadowing a generator assignment left the stale credit in place | `_ANY_ASSIGN` clears the credit | `E2` | | d | blanking a substitution to `" "` let a preceding value-taking option swallow the next word, so `--features $(...) my::filter::` lost its filter entirely | emit an `@@sub@@` sentinel on **both** the inline `$( )` and `$var` paths | `D1`, `D1b` | | e | `llvm-cov` and `+toolchain` invocations were not scanned | first-token rule, so `Install cargo-llvm-cov` steps are examined and correctly *not* flagged | `D2` | | f | a scanner that reads nothing reports a clean pass | assert the scan reached `ci.yml` and names the required lane's own test step | `F1` | ### The (d) fixture is the finding worth recording My first version of the (d) fixture exercised the `$var` path. So the arm that mutates the **inline `$( )`** path — revert `@@sub@@` back to `" "` — came back: ``` D1 expect FAIL, got PASS *** DID NOT BITE *** ``` Right colour on every other arm, and this one silently inert: the fixture never reached the code the mutation changed. The generalisation is the mirror of Gaff's rule about naming what you expect to be red: > **A mutation that does not bite is a statement about your fixture, not about your code.** It does not mean the path is safe; it means nothing you wrote goes there. That is a third distinct way a battery lies, alongside the two already on the board — Gaff's zero-selection filter, and Gaff's 18-tests-none-of-them-the-right-ones. All three produce a number that looks like a measurement. With the inline fixture added, both arms bite: ``` D1 expect FAIL, got FAIL failing: MUTATION: the detector finds an unguarded filtered step D1b expect FAIL, got FAIL failing: MUTATION: the detector finds an unguarded filtered step F1 expect FAIL, got FAIL failing: ...and the scan actually reached ci.yml (not a pass by reading nothing) ``` ### Direction-of-error, restated for these two scanners They fail in opposite directions and the asymmetry drove every one of the six repairs: - `test_step_test.sh` enforces a **ban**. A false positive fails loudly on a benign name. That is the correct direction and I did not loosen it — the same argument Gaff made against word-boundarying `shape_dispatch_gate`. - `packages_tested_by` grants **coverage credit**. Here an over-read is *fatal and silent*: it says a package is linted when nothing lints it. So it credits only variables it can prove are referenced, and stops at blank lines, at lesser indent, and at a step boundary. Findings (a), (b) and (c) are all the same species — an over-read on the credit-granting side — which is why they are the ones I would have shipped without noticing. ### Local state, re-measured on the current base ``` verify exit 0 verify-required-tier exit 0 self-test 51/51 arms behaved as stated test_step_test.sh 38/38 assertions passed ci.yml 9 guards / 9 assignments / 0 unbound substitutions / 9 jobs job names byte-identical to main shellcheck clean apart from the SC1091 already on main ``` `main` was 8 commits ahead when I finished, one of them touching the crates this branch's lanes build. Because `strict_required_status_checks_policy = false`, CI would otherwise have graded me on the stale base — so I merged `main`, re-ran everything above, and confirmed the merge touched **none** of `ci.yml`, `workspace_test_packages.py` or `test_step_test.sh`, so the mutation battery transfers unchanged. **#2142 is fixed on `main`** by #2147, which clears the `Rust (Windows ARM64)` and `Rust coverage (macOS arm64)` reds listed above. **#2149 remains open** and is not mine; my diff still contains zero Rust files. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Holden <holden@users.noreply.github.com>
1 participant
Closes #2058.
21 crates are compiled and tested by CI and linted by nothing. This derives the clippy package lists from the same source the test lanes already use, and adds a guard so the two cannot drift apart again.
The correction I owe first
#2058 claimed two live
clippy::unnecessary_castdenials onmaininonnx-runtime-ep-plugin/src/compute.rs. They are gone, and I did not fix them. Atbc715c329:git log 67d3aa5eb..HEAD -- crates/onnx-runtime-ep-plugin/is three feature/refactor commits (#2049, #2064, #2083); none mentions clippy or the cast. The lines were deleted incidentally by a refactor.That weakens the issue's headline and strengthens its actual point. The gap admits defects and releases them unobserved — nobody knows what is in there at any moment without running the lint themselves. The sample was never the argument; the mechanism is.
The rule that created the gap was false
ci.ymlinstructed: "To add a crate, first confirm its normal+dev dependency tree contains no ort-sys/CUDA dependency."That rule was already violated by the list it annotated. Measured:
And it does not matter which way you resolve that, because
cargo clippyonly ever checks — it never links and never runs a test binary. The offline/ort-backed split exists forcargo test; it has no force for lint.onnx-genai-ort-sysitself compiles here with no network.In #2058 I wrote that I did not know whether the premise was stale or whether
ep-pluginshould be out ofoffline-linux, and would rather flag it than guess. This is the measurement I said I would not substitute a guess for: the premise was false.What changed
-plists — byte-identical to each other, 31 packages, repeated in three jobs — become$(python .github/scripts/workspace_test_packages.py cargo-args lint). The newlintlane is "every package some test lane compiles" (55). Windows ARM64 takesoffline-linux(49), still a strict superset of the 31 it linted before, and no ORT crates on that target.verifygains a lint-coverage half: it fails if any tested package is reached by nocargo clippyinvocation anywhere in.github/workflows. Generator calls are expanded, not skipped — a computed-plist counts, and a hand-written one cannot hide behind some other step computing one.Evidence
The guard detects the real defect. Run against unmodified
main, before theci.ymlchange, it reports the gap by name:The fix closes it, and the pass is not an empty selection. The exact command
ci.ymlnow runs:parsed with
--message-format=json: wanted 55, seen 55, MISSING: none. A clippy run that selected nothing would also exit 0, so the package set is confirmed present in the lint graph rather than inferred from the exit code.Mutations.
--simulate-unlinted onnx-runtime-ep-cpu--simulate-missing onnx-runtime-ir(pre-existing half, after refactor)offline-linuxonnx-genai,onnx-genai-capi,onnx-genai-ortcargo clippy -p onnx-genai ...added, lanes narrowedTwo defects the mutations found in my own work
Recording both, because in each case the check was passing at the time.
1. The scanner counted a YAML comment as an invocation. After I rewrote the explanatory comment — which contains the words
cargo clippy— the reported invocation count went10 -> 11. Nothing failed; the only symptom was a number moving that I had no reason to expect to move. A comment reading# cargo clippy -p foowould have grantedfoolint coverage. The scanner now skips comment lines, the count is back to 10, and the mutation table above has a cell for exactly this.2. My first self-test passed because
pythonwas not on PATH. It was written asif cmd ...; then fail; fi— a bare non-zero check. Command-not-found is127, which is non-zero, so it read as "the guard correctly failed". It passed loudest in precisely the case where nothing ran. It now requires exit 1 specifically, plus the matching failure message:This is the same shape as
QEMU_LD_PREFIXand| head: a weaker check fails in the direction of passing. It is also why the self-test step exists at all —--simulate-missinghad shipped with this guard since it was written and CI had never once invoked it. A control nobody runs is not a control.Scope and risk
Fast (Linux x86_64)andRust qualitygain 24 crates each. Verified locally: the whole 55-package set is clean at-D warnings --all-targets.Rust (Windows ARM64)gains 18 crates onaarch64-pc-windows-msvc, which I cannot run locally. If that lane goes red this is mine and I will narrow it before merging. That lane is not required, which — as of last night's two merged defects that green required checks did not catch — is exactly the kind of lane I intend to read rather than assume. I will wait for it.Normal
--squash --auto. No admin bypass.