ci: classify a PR by its own diff, not by everything main gained since

[justinchuby]

Closes #2104.

The defect

The changes job classified a PR with a two-dot diff against github.event.pull_request.base.sha. That SHA is the base branch tip at the moment the event fired, not the PR's merge-base — so the diff also reports every file main gained since the branch diverged, with the sign inverted, since the branch "lacks" them. A pure-markdown PR is then classified as code and takes the full matrix.

Found it by checking my own PR #2103, which changes exactly one .md file, and seeing all nine lanes queue.

range	files
git diff --name-only $BASE $HEAD (two-dot, before)	9
git diff --name-only $BASE...$HEAD (three-dot, after)	1
gh pr view 2103 --json files — ground truth	1

The eight extras are exactly #2091's file list — and base.sha is #2091's merge commit. (I first wrote #2080 here; see the review response in the comments for how that wrong citation was produced and verified away.)

It is intermittent, and that is the interesting part

My first inference was that the fast path essentially never fires. That was wrong, and I checked before writing it down. #2070 (three .md files) classified docs_only=true and skipped all nine lanes — its branch happened to be level with main when the event fired, so two-dot and three-dot agreed.

The trigger is precisely main gained a commit between the PR's merge-base and the event. Which means the fast path works whenever you go looking at it on a quiet tree, and silently doesn't on a busy one. Same shape as the stale-base problem: the answer depends on where main was standing, not on the PR.

Why it is worth fixing even though it is safe

The direction is fail-closed — more CI, never less — so this is cost and latency, not correctness. It matters because runner capacity is the binding constraint: queue depth has been in the 50s today, and a docs PR taking the full nine-lane matrix displaces work that actually needs it.

Safety

• push stays two-dot — before..after is the push itself, and three-dot would be wrong there.
• Failure behaviour is unchanged. If the merge-base is unavailable the diff errors, || true leaves files empty, and the existing elif [ -n "$files" ] leaves docs_only=false → full CI. fetch-depth: 0 is already set on this job.
• The docs-only CI classifier skips both required checks for markdown that is compiled into Rust #2077 guard (a .md compiled in by include_str! is source, not docs) is untouched and is arm C below.

Battery — 9/9

The step's script was extracted from the YAML and run under the runner's own shell (bash --noprofile --norc -eo pipefail), against real SHAs:

arm	case	old	new
A	docs-only PR, base advanced	false ← the bug	true
B	genuine code PR	—	false
C	pure .md edit to an include_str! target (#2077)	—	false
C2	pure .md edit, not an embed target — the control	—	true
C3	mixed .md + .rs	—	false
D	missing SHAs	—	false
E	unresolvable SHA	—	false
F	unknown event (schedule)	—	false
G	push event	false	false (unchanged)

C and C2 are the pair that carries the claim. Without C2, arm C passes on a classifier that answers "code" to literally everything. My first C2 fixture used docs/execution/CUDA_COVERAGE.md and the arm failed — that file is itself one of the two .md embed targets, reached by a relative path. The fixture was wrong, not the code. Worth stating because a control that fails looks exactly like a defect, and I nearly filed it as one.

Arm A's old column is the positive control for the harness: it proves the battery can produce a FAIL rather than agreeing with everything.

Expected CI on this PR

This PR changes ci.yml, so it is code — it must run the full matrix, and docs_only=false here is the correct answer, not a symptom. #2103 is the one that should flip to docs-only once this lands.

Requesting independent Opus review.

[justinchuby]

Independent Opus review: no MAJOR issues, and it independently re-derived the safety argument rather than taking mine. One MINOR, fixed in 8f011c937 (amended, force-pushed).

The MINOR was a wrong citation, produced by the exact command our own skill warns about

I wrote that the 8 extra files were #2080's. They are #2091's. Verified before accepting the correction:

base.sha 6caef22a5  =  feat(cuda): add batched cuFFT STFT (#2091)
the 8 extra files   =  #2091's file list, byte-identical
#2080 (9a07936c1)   =  an ancestor of the merge-base -> appears in NEITHER diff

base.sha is #2091's merge commit, which makes the corrected version not just right but the clearer explanation of the mechanism.

I got #2080 from git log -1 -- crates/onnx-runtime-ep-cuda/src/cufft.rs — which returns the file's last commit, not the one that introduced the eight paths. That is verbatim the failure mode documented in the last **Check:** of measurement-discipline §9 ("cite the commit that introduced a claim, not the last one to touch the file"), a Check added because review caught two wrong PR numbers produced the same way. Third instance. The instrument answered a question adjacent to the one I asked, and the answer was plausible enough to pass.

Corrected in all four places it had propagated to: the ci.yml comment, the commit message, issue #2104's body (with the derivation error named, so the next reader doesn't repeat it), and the PR body above.

Also took the NIT

The comment now cites #2103 for the observation and #2104 for the tracking issue, rather than blurring them.

What the review confirmed independently

Worth recording, because these were the load-bearing safety claims and they were re-derived rather than accepted:

• An advanced base.sha does not shift the merge-base — the reviewer constructed the case (advance main past the fork point, edit a file shared by both sides) and confirmed the merge-base stays at the divergence point and the shared file still appears. This was my main residual worry: that three-dot could hide a file changed on both sides. It cannot.
• Merge commits in the PR branch behave correctly — checked against docs(skills): a selector that resolved to one test may still be the wrong test #2103's real history, which does merge main forward.
• The working-tree embedded scan cannot open a hole — it only ever adds files to the treat-as-code set, so a tree/diff mismatch is fail-closed by construction.
• Whitespace-only SHAs stay fail-closed: git exits 128, 2>/dev/null || true swallows it, files is empty, docs_only stays false. The one shell case that could have produced a surprising non-empty list — an empty right operand, where git substitutes HEAD — is blocked by the existing [ -n "${HEAD_SHA:-}" ] guard.

No behaviour change from the fix, so the 9/9 battery in the description still stands; the amend touched a comment and the commit message only. Re-verified the YAML parses and the job set is still 9.

Marking ready.

[justinchuby]

Live evidence from this PR's own run, which is better than my local battery because it is the real runner, on the real base, with the changed workflow actually in effect.

Detect change scope on 8f011c937:

files embedded into Rust sources: 149
Changed files (pull_request):
.github/workflows/ci.yml
docs_only=false

One file, correct verdict, and the embed-count guard is healthy — no embed scan resolved no targets, no classifying as code (fail closed). So docs_only=false here is a real classification, not a guard firing, which is the distinction that matters when the alternative interpretation is "the instrument gave up".

The counterfactual, at the base the job actually ran on

Pulled the base out of the job's own Merge X into Y line rather than assuming it:

base the job ran against: d997e2961
  two-dot (old): 13 files
  three-dot (new, what ran): 1 file

The 12 extras span ep-api, ep-cpu, ep-plugin, two .textproto fixtures, hostlock.yml and a Python script — none of them in this PR. main advanced from c8509042a to d997e2961 in roughly the twenty minutes between my branching and the run, and every one of those commits would have been attributed to me.

Worth being exact about what this does and does not show: this PR's verdict is unchanged either way, because ci.yml is code and both branches of the comparison say false. It is a demonstration of the over-reporting, not of a verdict being corrected. The verdict correction is arm A of the battery, and #2103 is the case where it matters — it is a one-.md-file PR that should classify docs-only once this lands, and currently does not.

That is also the honest shape of the whole bug: it mostly does not change the answer, because most PRs contain code and the answer was already false. It changes the answer precisely for the PRs the fast path exists to serve.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.17%. Comparing base (bc715c3) to head (8f011c9).
⚠️ Report is 40 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2105      +/-   ##
==========================================
+ Coverage   80.32%   81.17%   +0.84%     
==========================================
  Files         426      429       +3     
  Lines      204769   214755    +9986     
  Branches   204769   214755    +9986     
==========================================
+ Hits       164483   174322    +9839     
+ Misses      34665    34664       -1     
- Partials     5621     5769     +148     

Flag	Coverage Δ
cli-ort-linux	72.51% <ø> (?)
cli-ort-windows	72.01% <ø> (-0.10%)	⬇️
mlas	85.93% <ø> (?)
offline	81.31% <ø> (+0.77%)	⬆️

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

🚀 New features to boost your workflow:

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