Repository navigation
fix(ci): markdown that is compiled into Rust is not a docs-only change - #2081
Conversation
`Detect change scope` classifies a PR as docs-only and skips BOTH required
checks, `Fast (Linux x86_64)` and `Rust quality`. Two markdown files under
docs/ are compiled into Rust with `include_str!`, so a pure markdown edit
to either can break `cargo test` while classifying as docs -- skipping the
checks that would catch it.
crates/onnx-genai-metadata/tests/capability_catalogue.rs:9
include_str!(".../docs/genai/INFERENCE_METADATA_DECISIONS.md")
crates/onnx-runtime-ep-cuda/src/kernels/mod.rs:1686
include_str!(".../docs/execution/CUDA_COVERAGE.md")
Falsified on 0ce253f, both edits touching only the .md (baseline 3/0):
rename an HTML comment marker -> panicked at capability_catalogue.rs:12:10:
capability catalogue start marker 2 passed; 1 failed
delete one catalogue table row -> assertion `left == right` failed:
update the normative capability catalogue... 2 passed; 1 failed
The second is the exact edit that test exists to police, so the test is
skipped by the class of change it was written for.
The property is not "these two paths are special", it is that a file whose
bytes are compiled into an artifact is source whatever its extension, so
the set is derived by scanning rather than listed and cannot go stale when
a third embed is added (Rule 10). Scan finds 143 embedded targets today, of
which exactly two are docs-classified. Only literal-path embeds are visible;
a computed path (concat!/env!) is not, and is documented as staying docs.
The scan is assigned inside `if` rather than as a bare assignment. Under the
default `bash -e` step shell a bare `v="$(git grep ...)"` exits the step when
grep matches nothing, and a dead `changes` job SKIPS fast-linux and
rust-quality -- and a skipped required check satisfies the ruleset. The
fail-open form of this fix is worse than the bug. Demonstrated:
bash -e -c 'v="$(grep zzz /dev/null)"; echo REACHED' -> rc=1, never reached
bash -e -c 'if v="$(grep zzz /dev/null)"; then :; ...' -> rc=0, reached
A `git grep` error (rc>1) forces docs_only=false, matching the classifier's
existing fail-closed-on-ambiguity contract.
Battery 9/9 plus 3 shell arms, driving the classifier extracted from ci.yml
via yaml.safe_load against real commits under `bash -e`. Four arms change
verdict against main's implementation, so the fix is not a no-op; the
unchanged arms (plain docs, LICENSE, rust source, mixed) confirm no
regression. Structural gate: job list identical to main, fast-linux and
rust-quality byte-identical, `changes` the only modified job.
Closes #2077
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Independent review found a fail-open in the guard I had just written, in the direction that matters: a docs-only edit to an embedded markdown file still skipped both required checks when the embed is written across two lines. `git grep` is line-oriented, so `include_(str|bytes)!\s*\(\s*"..."` only matches when the macro, the paren and the opening quote share a line. rustfmt wraps any call over 100 columns onto its own line, and 15 of this repo's 163 embed sites are already in that form. The two motivating .md embeds are 87 and 86 columns -- an unrelated reformat that pushed either past 100 would have silently reopened #2077, while the comment directly above the scan claimed it "cannot go stale". The guard's central claim was false. Isolated with a three-way control on one commit pair whose diff is exactly [docs/genai/INFERENCE_METADATA_DECISIONS.md], the embed wrapped on the base so it sits outside the diff: origin/main (no embed logic) docs_only=true the #2077 bug 79aa1ec (line-based scan) docs_only=true the fail-open HEAD (whole-file scan) docs_only=false fixed The scan is now whole-file via `perl -0777` over `git ls-files -z`, so the paren and the path may be on different lines. It also picks up raw-string forms (r"", br"", r#""#), and the tab-separated output removes the previous `file:line:match` parsing, which would have mis-split a path containing a colon. 158 hits vs 148, 149 resolved targets vs 143, still exactly two docs-classified -- so no docs PR starts running full CI unnecessarily. Second finding, same class: `embedded="$(printf | while | sort)"` was a bare assignment. The step declares `shell: bash`, which GitHub runs as `bash --noprofile --norc -eo pipefail`, so pipefail is ON and a failing iteration would have killed the step -- and a dead `changes` job SKIPS fast-linux and rust-quality, which satisfies the ruleset. I had guarded the first assignment against exactly this and left the second one open. Both are now `if !` with `embed_scan_ok=false`. Third: my battery ran the script under plain `bash -e`, not the real `-eo pipefail`, so it could not have observed either finding. Fixed; every new failure mode this patch introduces is a pipefail mode, so the harness was blind to precisely the class of defect the patch could add. Battery 10/10 under the real shell flags, 5 arms changing verdict against main. Structural gate unchanged: 9 jobs, list identical, fast-linux and rust-quality byte-identical, `changes` the only modified job. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Independent Opus review returned REQUEST CHANGES and found a fail-open in the guard I had just written — in the direction that matters. Reproduced all three findings before fixing. MAJOR — a two-line embed was invisible, so #2077 stayed open
The two motivating Isolated with a three-way control on one commit pair whose diff is exactly The middle row is the finding. My original Scan is now whole-file via MINOR — I guarded one assignment against pipefail and left the next one openThe step declares I had written a comment on the assignment immediately above explaining this exact hazard, then left the next assignment unguarded. Both are now Reviewer was straight that they could not make MINOR — my battery could not have observed either findingIt ran the script under plain Structural gate unchanged: 9 jobs, list identical to Accepted as documented limitationsRaw strings now handled; computed paths ( Thanks — the MAJOR is a genuine fail-open that my own controls were structurally incapable of detecting, which is the most useful kind to have found. |
… zero The embed scan added in this PR closes #2077, but it had no control proving it can fire. If the regex, the pathspec or perl ever stops matching, the resolved set is empty, `is_docs_path` never blocks, and the guard is inert -- docs-only edits to embedded markdown would again skip both required checks, behind a completely green run. A scan that found nothing and a scan that cannot find anything were the same two lines of log. So the step now echoes the number of resolved targets, and treats zero as a broken instrument rather than as an answer: this repo has 149 such targets and cannot legitimately reach zero while any `include_str!` remains. Controls, 5/5, with the positive control run first so the zero is attributable: A embedded .md edited -> docs_only=false, count=149, no alarm B non-embedded .md edited -> docs_only=true, count=149, no alarm (not over-broad) D synthetic WITH include_str! -> count=1, no alarm (proves the harness can produce a nonzero count) C synthetic with no .rs -> count=0, alarm, fail closed E same tree under main's classifier -> docs_only=true (discriminator) Original battery still 10/10, 5 arms changing verdict vs main; job list identical to main and fast-linux/rust-quality byte-identical. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pushed The Both produce That is exactly the failure mode I merged in #2070 §9: a scan that found nothing and a scan that cannot find anything are the same two lines of log. I had shipped a detector with no control proving it can fire. So the step now echoes the resolved count and treats zero as a broken instrument rather than as an answer — this repo has 149 such targets and cannot legitimately reach zero while any Controls, 5/5 — the positive control runs first, so the zero in arm C is attributable to absence rather than to a dead harness:
Original battery still 10/10, 5 arms changing verdict vs Diff is +18 lines, additive only. Re-running full CI from scratch. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2081 +/- ##
==========================================
+ Coverage 80.27% 80.60% +0.33%
==========================================
Files 422 428 +6
Lines 198030 209200 +11170
Branches 198030 209200 +11170
==========================================
+ Hits 158964 168627 +9663
- Misses 33474 34879 +1405
- Partials 5592 5694 +102
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…rong test (#2103) ## What §9 of `measurement-discipline` guards selector **cardinality** — `--list | grep -c ': test$'` must resolve to exactly 1, and the run output must then read `1 passed`. Both checks answer *how many tests ran*. Neither answers *whether they were the tests that cover the mutated code*, and a mutation battery only carries its claim if the answer to the second is yes. This adds the distinction, the falsifier, and a `**Check:**`. ## The gap, measured Purpose-built two-test crate; filter `--exact` onto a test that does not touch the mutated function: ``` precheck n = 1 (the -eq 1 guard is SATISFIED) mutate covered(a) -> a + 1 becomes a + 99 arm expects FAIL, gets 1 passed; 0 failed -> reads "survived" control unfiltered 1 passed; 1 failed -> the mutant IS caught ``` The guard never fires. The arm reports a clean PASS over a live mutant. Worth being precise about what §9 already covers, since this was my first reading and it was wrong: §9's guard is `-eq 1`, **not** `>= 1`, so it *does* reject the 18-test case on cardinality alone. The residual gap is narrower and nastier — **identity, not cardinality**. When the selector resolves to exactly one test and that one is wrong, every existing check in the section passes. ## Provenance #2000 (closing #1995) hit the same shape at a larger count: a substring filter selected 18 tests, none covering the arm under test, and reported PASS. A non-zero selection *suppresses* the suspicion an empty one would raise, which makes it strictly harder to catch than the vacuity case already documented. I verified the load-bearing half from the tree rather than relaying it: the three tests that do cover that arm exist, and none of their names contains the filter word. The count `18` is attributed to that battery's own output and labelled as such — I did not rebuild the server crate to re-derive it, and the text says so rather than implying I measured it. ## The snippet is executed, not asserted The `**Check:**` ships a snippet, so it was run in both directions plus a firing control: | case | result | |---|---| | wrong filter, mutation live | `ARM-DRIFT`, exit 2 ✅ | | right filter, mutation live | `arm OK`, exit 0 ✅ | | right filter, **mutation reverted** (control) | `ARM-DRIFT` ✅ — the check fires when there is nothing to catch | Without the third row the first two prove only that the command runs. ## Scope Docs only — one file, +38/-1. No code, workflow, or test behaviour changes. The frontmatter `source:` gains `#1995/#2000 selector identity`, matching the existing `#1619/#1982 selector vacuity` form. Incidental confirmation while building the falsifier: `cargo test --lib <bare_name> -- --exact --list` on a test inside `mod tests` printed `0 tests`, exit 0 — §9's own opening claim, reproduced independently. ## Expected CI This should classify **docs-only** and skip both required checks, which also makes it a live exercise of the classifier merged in #2081. I will confirm the skip comes from correct classification rather than from the fail-closed guard before merging. Requesting independent Opus review. --------- Co-authored-by: Holden <holden@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A skipped required check satisfies a ruleset. Every job in ci.yml carries `if: needs.changes.outputs.docs_only != 'true'`, so anything that makes the `changes` classifier emit docs_only=true skips both required checks and the PR is mergeable -- with the tests present, unrun, and reported as neither pass nor fail. Holden falsified the reachable case in #2077: two .md files are compiled into Rust via include_str!, so a pure-markdown edit can skip both required lanes on a tree that does not compile (#2081 fixes the classifier). This gate reasoned about step-level `if:` and stopped there, one level below where the guarantee actually lives: it proved the ORT step runs whenever the job runs, and said nothing about whether the job runs. Pin the required jobs' conditions to the known form instead, and refuse an unrecognised one. Five self-test arms, positive control first so that a zero is attributable to the condition being acceptable rather than to a harness that cannot refuse. The last arm is the discriminator: a step-level `if:` is indented deeper and must not be read as the job's -- without it, a regex matching any `if:` would pass every other arm, because no fixture would tell them apart. Mutation-proved end to end on the real workflow in both directions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Closes #2077.
Detect change scopeclassifies a PR as docs-only and skips both required checks —Fast (Linux x86_64)andRust quality:Two markdown files under
docs/are compiled into Rust withinclude_str!, so a pure markdown edit can breakcargo testwhile classifying as docs, skipping the checks that would catch it.Falsified, not argued
Both edits below change only the
.md—git diff --name-onlyreturns one path, sodocs_only=true. Baseline on0ce253f4ais3 passed; 0 failed.Rename an HTML comment marker. The test locates its table with
.expect(...):Delete one catalogue table row:
The second is the exact edit that test exists to police. The test is skipped by the class of change it was written for.
The fix is derived, not a special case
The property is not "these two paths are special", it is a file whose bytes are compiled into an artifact is source, whatever its extension. So the set is scanned, not listed, and cannot go stale when a third embed is added (Rule 10):
then resolved relative to each including file with
realpath -m --relative-to. 143 embedded targets today, of which exactly two are docs-classified. A hardcoded two-path exception would be correct today and silently wrong on the third embed — the same shape as the.gitignorepattern list that #2036 replaced after four consecutive predictions failed.Documented limitation: only literal-path embeds are visible. A computed path (
concat!/env!) is not, and stays classified as docs.The dangerous way to get this wrong, which I hit in my own patch
My first draft used a bare assignment. Under the default
bash -estep shell,v="$(git grep ...)"exits the step when grep matches nothing — and a deadchangesjob skipsfast-linuxandrust-quality, and a skipped required check satisfies the ruleset. The fail-open form of this fix is worse than the bug it fixes.A
git greperror (rc>1) forcesdocs_only=false, matching the classifier's existing fail-closed-on-ambiguity contract. I did not change that contract; the file is careful about it and says so.Validation
The battery drives the classifier extracted from
ci.ymlviayaml.safe_load— never a text split, because #2052 shipped a battery that text-matched a job name and therefore could not distinguish a job from a key nested inside another job — against real commits, underbash -eto match GitHub's step shell.Every arm is paired with main's implementation as the control, so an arm cannot pass by the harness failing to exercise anything — the four
<-- changedrows are the proof the battery has power.Blast radius
This edits a required workflow. An invalid workflow does not fail its checks, it removes them — observed on #2052, where a four-space indent made one job a key inside another and the PR showed 14 green with both diff-guard checks simply absent, satisfying
pending=0 && failed=[]. That is why the structural gate above parses the YAML and diffs the job list rather than reading the diff.changesis the only modified job; both required jobs are byte-identical tomain.Note this PR is not docs-only, so it exercises the full required set on itself.