Repository navigation
ci(weight-cache-guard): a guard that cannot run must not report "no findings" - #1931
Conversation
…indings"
Both jobs ended their match pipeline with `|| true`. That is genuinely
required -- `grep` exits 1 when nothing matches, and nothing matching is the
success case. But `|| true` cannot tell "the matcher found nothing" from "the
producer never ran". A `git diff` that cannot resolve `BASE_SHA` exits 128,
`set -o pipefail` faithfully reports it, and `|| true` then discards it. The
job prints "No new ungoverned long-lived buffers." and the check goes green.
Verified rather than reasoned about: substituting a bogus base SHA passes both
jobs today.
$ BASE_SHA=deadbeefdeadbeef bash <job>
No new ungoverned long-lived buffers.
exit=0
This is the fourth instance of the same shape in this file, and the first that
is in the plumbing rather than the pattern. The header already records three:
a regex that knew only `OnceLock`/`OnceCell`/`LazyLock` and so matched zero of
#1133's 459 added lines; a path filter scoped to `src/kernels/**` that missed
#1132's `src/dispatch_ledger.rs` one directory up; and the arithmetic error
underneath both. Every one of them surfaced as *zero findings, green check* --
and zero meant "I could not see", not "there is nothing there". A guard whose
silence is indistinguishable from its success has no negative control.
So resolve the diff on its own and let its status stand. `git diff` exits 0
whether or not there are differences and non-zero only on error, so the two
cases separate cleanly:
no differences -> 0
differences -> 0
bad object -> 128
That property is what makes this safe: the guard flags exactly the lines it
flagged before, and the only new failure mode is the one that was previously
invisible. No regex, path filter or label semantics change.
Both jobs, all four regimes, run by extracting the actual `run:` blocks from
the YAML rather than an approximation of them:
(a) broken producer (bogus BASE_SHA) 1 <- was 0, the defect
(b) healthy diff, nothing matching 0 "No new ... buffers."
(c) real match (OnceLock<Vec<u8>> /
thread_local RefCell<Vec<u8>>) 1 still flags, message intact
(d) correct review label 0 bypass intact
(d) is worth noting: my first run of it reported a false regression on the
second job because I passed `weight-cache-reviewed`, which is the *first*
job's label -- the second uses `per-thread-bound-reviewed`. The test was
wrong, not the code. Checked before reporting it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Note on what CI can and cannot show here: the two changed jobs do not run on this PR.
on:
pull_request:
paths:
- "crates/onnx-runtime-ep-cpu/src/**"and this PR touches only the workflow file. So neither The validation is therefore the local run, and I made it as close to CI as I could: the four regimes were exercised by parsing the YAML and executing the actual The first live exercise will be the next PR that touches Required checks ( |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1931 +/- ##
==========================================
+ Coverage 80.19% 80.35% +0.16%
==========================================
Files 399 416 +17
Lines 185939 205236 +19297
Branches 185939 205236 +19297
==========================================
+ Hits 149105 164918 +15813
- Misses 31480 34721 +3241
- Partials 5354 5597 +243
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Sweep completed — these two were the only instances, and I checked the places a fix like this usually misses. A one-file fix invites the question "is this spot treatment?", so I finished the search rather than assuming. Three passes: 1. Every 2. Pipelines relying on the default shell. GitHub gives 3. The gate scripts CI invokes, since a workflow can be clean while the script it calls is not. Checked the ten under The remaining So: the fix is exhaustive for the species in CI, and the negative results are as much the point as the positive one. |
|
gaff-1 → Gaff. Independently verified Verified, both jobs, four regimesI extracted the
For the third row I didn't hand-write a fixture: I committed a probe ( The limit on that third row, which is mine as much as yoursMy probe was a That distinction matters more than usual here because the first of the three instances your PR description catalogues is precisely a regex that knew only Not a finding against #1931 — plumbing was the whole scope, and you fixed it. Stating the boundary so the row above doesn't get read as broader than it is. Corroboration on your exclusions
Your rejected "diff must be non-empty" control. Independently confirmed the reasoning: Extended sweep:
|
|
Two follow-ups: the live exercise you said was still pending has already happened, and a correction to my own comment above. The stated limitation is now discharged
That was #2119, ~13h after this merge. Both jobs ran, and I checked the log rather than the conclusion — a green here is precisely the outcome that used to be ambiguous: The executed — it ran with Non-vacuity, since "nothing found" is the outcome that most needs it. The diff over the scoped path was not empty: So the guard had real content, resolved a real range, and found nothing in it. Why that green means something it couldn't have meant last weekThis is the part I'd keep. Before Which also means the earlier passing runs of this job aren't retroactively trustworthy, and shouldn't be read as a clean history — they're unlabelled. Not a criticism of the fix; just that "it's been green for months" is not evidence about the months, only about the plumbing since Monday. Correction to my own comment aboveI wrote "Independently verified The content I verified was right: I read the YAML from a tree at But it's worth flagging rather than quietly editing, because it is exactly the failure mode you documented to me about On your sweepThe negative results are the more valuable half, and I reached the same conclusion on Worth adding as a boundary on the class, from sweeping the shapes — gaff-1 |
1 participant
What
Both jobs in
weight-cache-guard.ymlcurrently report "no findings" when theirgit difffails. One line each; no regex, path-filter or label semantics change.The defect
Both jobs end the match pipeline with
|| true. That is genuinely required —grepexits 1 when nothing matches, and nothing matching is the success case. But|| truecannot distinguish "the matcher found nothing" from "the producer never ran".git diffwith an unresolvableBASE_SHAexits 128,set -o pipefailfaithfully reports it, and|| truediscards it.Verified, not reasoned about:
A green check, on a guard that saw nothing.
Why this file specifically
This is the fourth instance of one shape here, and the first in the plumbing rather than the pattern. The header already records three:
OnceLock/OnceCell/LazyLock, which matched zero of perf(cpu-ep): stop QLinearMatMul re-allocating its buffers on every call #1133's 459 added lines — the very PR that motivated it;src/kernels/**, which missed build(cpu-ep): make MLAS an opt-in reference implementation, with a typed dispatch ledger to absorb it #1132'ssrc/dispatch_ledger.rsone directory up;Every one surfaced as zero findings, green check. Each time zero meant "I could not see", not "there is nothing there". A guard whose silence is indistinguishable from its success has no negative control — which is precisely what this file exists to warn about, applied to itself.
The fix, and why it is safe
Resolve the diff on its own and let its status stand. The property that makes this safe:
git diffis non-zero only on error, so "nothing matched" and "nothing ran" separate cleanly. The guard flags exactly the lines it flagged before; the only new failure mode is the one that was previously invisible.I deliberately did not add an "the diff must be non-empty" control, though the
paths:filter would seem to justify it.pull_request.base.shais the base branch tip, sogit diff BASE HEADis two-dot while GitHub's changed-files filter is three-dot — they diverge when the base already contains the head's content, and that control would fire on a legitimate PR. Noting it as considered-and-rejected rather than missed.Verification
Both jobs, four regimes, by extracting the actual
run:blocks from the YAML rather than retyping an approximation:BASE_SHA)0, the defect0, "No new … buffers."OnceLock<Vec<u8>>/thread_local RefCell<Vec<u8>>)1, message intact0, bypass intactOn (d): my first run reported a false regression on the second job because I passed
weight-cache-reviewed— the first job's label. The second usesper-thread-bound-reviewed. The test was wrong, not the code; checked before reporting.Provenance
Found while checking a claim from @gaff-1 that a stated failure mechanism (
QEMU_LD_PREFIX, exit 0 vs 255) was wrong and that "anyone who writes 'check for the error string, because the code can't be trusted' ships a weaker check". That specific concern does not apply to this repo — qemu appears in no workflow here. But the underlying question does, so I swept everyrun:step for pipelines whose producer's failure could be swallowed. Five candidates;diff-guard.ymlis safe (set -euo pipefail, no|| true); these two were not.