fix(ci): compare the ABI against the commit under test, and give Windows CI a permissions block

[Alexandr-Solovev]

Description

Two independent CI findings, both surfaced by CodeQL on the Actions workflows, plus a follow-up that takes cache access away from Nightly-test (see the end of section 2).

1. ci-win.yml had no permissions: block

It was the only workflow in .github/workflows/ without one, so its push: main runs received the repository-default token for a job that only builds and tests. Set to contents: read.

2. The ABI check could compare a pull request against a tree that already contains its change

abidiff reports the difference between two trees, not what a pull request changed. For the report to equal the pull request's own delta, the baseline has to be an ancestor of the build under test. Main's tip is not one: checkout builds the pull request's merge commit, which contains main only up to the pull request's base.

How a real break passes:

1. main at M0. Pull request B is opened from M0 and removes exported symbol S.
2. Pull request A removes S too -- a re-land, a cherry-pick, or the other half of one refactor. A merges carrying API/ABI breaking change, so A's own ABI check never ran (the job's if: skips it on that label).
3. main is now M1 = M0 + A, and its cache lands. B's baseline becomes M1, because the baseline was selected as the newest entry in main's cache scope.
4. B's artifact is a build of M0 + B. abidiff(M1, M0+B): S is absent from both sides, so nothing is reported. B goes green and merges without the label.
5. The next pull request's baseline is M1 + B. B's break is part of the baseline from then on and no run will ever see it.

The nastier variant does not need the two changes to be identical. If A changed a struct layout (labelled, accepted) and B independently removes a method from that struct, abidiff reports B's removal plus A's layout change inverted, and the job's own help text trains the reader to attribute unexplained diffs to baseline drift: "Keep the branch up to date with oneDAL 'main' as it may otherwise not contain the latest changes in the ABI that can lead to erroneous failures." Once drift is an accepted excuse, a real finding is dismissed along with the noise.

Fix. Start from the first parent of the merge commit -- the main commit the build actually contains -- and walk its ancestors for a cached build. The baseline is then always an ancestor of the build, so main running ahead adds no drift. When the base commit itself is cached (the normal case), the diff contains only the pull request's own changes and nothing is masked. When the base's main build has not been cached yet, the newest cached ancestor is used: the main commits in between show up in the report, a change that undoes one of them is not reported, and the job says so in a ::warning:: (rerun the job once main's CI for the base has finished).

Selection moved to .ci/scripts/abi_baseline_key.sh; the workflow step is one line.

Also changed by the same rewrite: the cache listing no longer picks the baseline

The old selection was gh cache list --sort created_at --limit 1, so whatever key was written to main's cache scope last became the baseline. Only trusted triggers (push, schedule, workflow_dispatch) can write there; workflow_run, pull_request_target and issue_comment runs get read-only access. Nightly-test's workflow_dispatch runs did have write access, and this PR now sets cache-mode: none on that workflow (cache-mode: read for cve-bin-tool-scan). cache-mode: none replaces the per-step blanking of ACTIONS_RUNTIME_TOKEN, ACTIONS_RESULTS_URL and ACTIONS_CACHE_URL, so those env: blocks are removed from the bazel_lnx and bazel_win steps, and the file's Cache access note is rewritten to match.

Candidate keys are now derived from commits and only looked up in the cache listing, never read out of it. A writer could still re-create the key of a real main commit whose entry has expired, and the walk would pick it if nothing newer is cached; the restored tree is only ever read by abidiff. Ordering by commit position also fixes a staleness bug in its own right: a cache created recently for an older main commit used to win over main's tip.

Failure modes are all loud

What goes wrong	What happens
No entry for the base or its last 20 ancestors	falls back to main's tip with a ::warning:: naming what the comparison can then miss
No entry on main at all within the window	::error:: and exit 1
Key found but cache/restore cannot read it	fail-on-cache-miss: true, unchanged
Restored but the tree is empty or partial	abi_check.sh prints ::error:: No shared objects found and exits 1, unchanged
Empty base commit passed	::error:: and exit 1 (a caller bug, not a cache gap)
gh cache list or the commit lookup fails	::error:: naming the failed call, "rerun this job", exit 1
Baseline any number of commits behind the base	::warning::, so an inexact comparison is never silent

Not covered here

Nothing checks the cumulative ABI delta against the last release. A labelled pull request is never checked and is then absorbed into every later baseline, so two breaking changes mean two labels and no gate that ever adds them up. That needs a released .so as the baseline rather than a main cache, which is a separate job.

Validation

.ci/scripts/abi_baseline_key.sh was exercised against main's real ancestry with a stubbed gh, covering: base cached exactly (0 back, silent); base cached 1+ ancestors back (warning); base uncached with only main's tip cached (falls back, warns); a poisoned invented key present alongside a valid one (ignored); a poisoned key only (hard error); empty cache listing (hard error); empty base (hard error); failed cache listing (hard error). All three changed workflows parse as YAML, cache-mode is placed at workflow and job level as the dependency caching reference allows, and the script passes bash -n and shellcheck.

This pull request's own ABI Conformance(avx2) run picked __release_lnx-500d5ac (exact: yes, 0 commits back), the first parent of its merge commit.

I also confirmed against the live repository that the premise holds and the window is generous: 15 live __release_lnx entries covering every one of main's last 15 commits, 50 MB each, so there is no eviction pressure and the only gap is the 7-day expiry.

---

Checklist:

Completeness and readability

• I have commented my code, particularly in hard-to-understand areas.
• I have updated the documentation to reflect the changes or created a separate PR with updates and provided its number in the description, if necessary.
• Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
• I have resolved any merge conflicts that might occur with the base branch.

Testing

• I have run it locally and tested the changes extensively.
• All CI jobs are green or I have provided justification why they aren't.
• I have extended testing suite if new functionality was introduced in this PR.

[Copilot]

Copilot review overview

🟡 Changes recommended

A failed base-commit API lookup can silently select an inexact baseline instead of failing the ABI check.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Fail loudly when the base API lookup errors · New
• Enable strict Bash mode with set -euo pipefail · New

What changed in this PR

Hardens CI permissions and makes ABI comparisons use a cached ancestor of the tested commit.

Changes:

• Restricts Windows CI to read-only repository contents.
• Adds ancestry-aware ABI baseline selection.
• Ignores untrusted or stale cache-key ordering.

File	Description
.github/​workflows/​ci.yml	Uses the baseline-selection script.
.github/​workflows/​ci-win.yml	Adds restricted token permissions.
.ci/​scripts/​abi_baseline_key.sh	Selects and reports cached ABI baselines.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Alexandr-Solovev]

Agreed, and it was worse than a miss: for sha in $(gh api ...) iterates zero times when the lookup fails, and set -e does not catch a failed command substitution in a for list. So a 502 looked exactly like "no cached ancestor" and the fallback picked main's tip -- the non-ancestor baseline this whole change exists to avoid.

Fixed in c8d0188 by taking the API calls out of the loop. ancestors_of is now assigned to a variable at the top level, so a failure is a distinct status the caller stops on:

base_commits=$(ancestors_of "${BASE_SHA}") || {
    echo "::error::Could not list the commits reachable from ${BASE_SHA}. Rerun this job."
    exit 1
}

newest_cached_in is pure string matching now, so "no candidates" can only mean no candidates. Added three tries with a 0/5/10s backoff so a single hiccup does not turn into a red check.

The cache listing keeps carrying on past a failure, and that asymmetry is deliberate: an empty listing cannot match any key, so the job stops at the existing "No __release_lnx cache on refs/heads/main" error instead of comparing against nothing. Failing there is safe; failing in the commit lookup was not. Both directions are covered by the offline dry run -- an injected 502 on the base lookup now exits 1 where it previously printed exact: no and exited 0.

[Alexandr-Solovev]

Right, AGENTS.md asks for set -euo pipefail and I shipped -eo. Changed in c8d0188.

The script was already written to survive -u -- the optional inputs use ${1:-} and ${GITHUB_OUTPUT:-} -- so the only behaviour change is the one you point at: with GITHUB_REPOSITORY unset the lookup now dies with GITHUB_REPOSITORY: unbound variable at the point of use instead of requesting repos//commits?sha=.... Verified by running the script with that variable unset.

Worth noting the other three scripts under .ci/ (build.sh, env/openblas.sh, env/tbb.sh) are all on set -eo pipefail. Leaving them alone here since adding -u to them is not this PR's subject.

[napetrov]

The ci-win.yml permissions block looks right: no job there calls gh, uses the cache actions, or needs token scopes beyond reading contents. That closes code-scanning alert 17.

The ABI baseline change has one blocking problem: as wired up in ci.yml, the script never receives the base commit, so every run takes the fallback path. This PR's own ABI Conformance(avx2) run shows it:

##[warning]No cached main build for this pull request's base or its last 20 ancestors, so the baseline is main's tip. ...
__release_lnx-82acef1606a33d34cfc22116267d73eab4d2f579 (exact: no, 0 commits back)

That cache entry exists, and it is the merge commit's first parent, so the exact path should have matched. Details and a fix are inline. There are two smaller points too: a baseline walked back past the base can still hide a removal (reproduced with abidiff), and the "Provenance" rationale does not match GitHub's current cache rules.

Note: this PR does not touch nightly-test.yml, so code-scanning alerts 15/16 stay open after it merges.

[napetrov]

actions/checkout fetches with --depth=1 (this run's log: fetch --depth=1 origin +8198e87…:refs/remotes/pull/3838/merge). In a shallow clone git treats the boundary commit as parentless, so git log -1 --format=%P prints an empty string and the script is called with "".

I reproduced this with the same fetch as the job:

$ git fetch --depth=1 origin +refs/pull/3838/merge:refs/remotes/pull/3838/merge && git checkout --detach refs/remotes/pull/3838/merge
$ git log -1 --format=%P                               # -> ""   (what the PR passes)
$ git rev-parse HEAD^1                                 # -> fails, "HEAD^1"
$ git cat-file -p HEAD | awk '/^parent /{print $2; exit}'   # -> 82acef1606a33d34cfc22116267d73eab4d2f579

Then I ran the script from this PR against the live cache listing with each input:

abi_baseline_key.sh ""        -> __release_lnx-51b69f23… (exact: no, 0 commits back)  + fallback warning
abi_baseline_key.sh 82acef16… -> __release_lnx-82acef16… (exact: yes, 0 commits back)

The commit object stores its parents even when the clone is shallow, so reading it directly works:

Suggested change

	.ci/scripts/abi_baseline_key.sh "$(git log -1 --format=%P | cut -d' ' -f1)"
	.ci/scripts/abi_baseline_key.sh "$(git cat-file -p HEAD | awk '/^parent /{print $2; exit}')"

${{ github.event.pull_request.base.sha }} would also work: for this run it equals the merge commit's first parent (82acef16…), and re-runs reuse the same payload. cat-file has the advantage of reading the exact commit that was built.

[Alexandr-Solovev]

Fixed by your 9e66d78. I reviewed it and agree; the description is updated to match.

[napetrov]

An empty BASE_SHA silently falls through to the "no cached main build for this pull request's base" warning. That message is wrong for this case, and it is the reason the bug above passed CI unnoticed. In pull_request runs an empty base is always a wiring bug, not a cache gap. Suggest failing fast instead:

if [ -z "${BASE_SHA}" ]; then
    echo "::error::No base commit was passed. The caller must pass the first parent of the merge commit under test."
    exit 1
fi

(This would change the "non-merge HEAD falls back and warns" case from the validation list. If that case is still wanted, it should at least print its own message.)

[Alexandr-Solovev]

Fixed by your 9e66d78. I reviewed it and agree; the description is updated to match.

[napetrov]

The description says the diff will contain "only the pull request's own changes: no spurious failures from drift, and no masking either". That holds only when the match is 0 commits back. If the base itself is not cached, the walk picks an older ancestor. The diff then also contains the main commits in between, and a pull request that undoes one of them is hidden. I reproduced this with abidiff (libabigail from Ubuntu 24.04):

• M0: exports f
• M1 = M0 + adds S (an addition, so no label needed)
• PR on M1 removes S

abidiff libM1.so libPR.so  -> "1 Removed function: int S()"   rc=12  (abi_check.sh fails)
abidiff libM0.so libPR.so  -> (no output)                      rc=0   (abi_check.sh passes)

This window comes up regularly: on current main, the __release_lnx-<sha> entry is created about 70 minutes after the commit lands (51b69f2: committed 13:25Z, cached 14:34Z; 82acef1: 16:39Z → 17:49Z). Any PR run in that window matches 1 commit back, and with STALE_AFTER=5 it gets no warning at all.

Suggestions, in order of preference:

• treat behind > 0 as ::error:: ("baseline for is not cached yet, the main CI run is probably still in progress; re-run this job later"), or
• at least default STALE_AFTER to 0, so any inexact match is visible.

The description should also be adjusted to say that the masking guarantee only holds for an exact match.

[Alexandr-Solovev]

Fixed by your 9e66d78. I reviewed it and agree; the description is updated to match.

[napetrov]

Any job whose GITHUB_REF is main can write into main's cache scope, the fork pull request jobs in Nightly-test included

This no longer matches GitHub's documented behaviour. Runs triggered by workflow_run (and pull_request_target, issue_comment) get read-only access to the default branch's cache unless the workflow declares a write-capable cache-mode. Only push, workflow_dispatch, repository_dispatch, schedule, delete, registry_package and page_build can write to it (dependency caching → Cache access for low-trust workflow triggers). CodeQL's cache-poisoning query models the same split: it flags nightly-test.yml only for its workflow_dispatch trigger.

Deriving keys from commits is still the right design, so no code change is needed, but the comment should say what it actually defends against. Also, "keys are immutable, so the only race left is against the real main build" does not cover keys for older main commits whose entries have expired (7-day TTL) or been evicted. Whoever can write could plant one of those, and the walk would pick it if nothing newer is cached. The impact is bounded either way, since the restored tree only feeds abidiff as data.

[Alexandr-Solovev]

Fixed by your 9e66d78. I reviewed it and agree; the description is updated to match.

[napetrov]

If gh cache list fails three times, available="" and the job ends with "No __release_lnx cache on refs/heads/main … Rerun the 'CI' workflow on main via workflow dispatch". Failing is correct, but that advice is wrong: re-running main's CI won't fix an API outage. Checked with a stubbed gh that returns HTTP 502 for cache:

HTTP 502: Bad Gateway   (x3)
::error::No __release_lnx cache on refs/heads/main for any of its last 20 commits. Rerun the 'CI' workflow on main via workflow dispatch to regenerate it.

Suggest giving the listing failure its own ::error::Could not list caches … rerun this job, the same way ancestors_of failures are handled.

[Alexandr-Solovev]

Fixed by your 9e66d78. I reviewed it and agree; the description is updated to match.

[napetrov]

Follow-up on code scanning, separate from the ABI review above.

This pull request closes alert #17. I ran CodeQL 2.27.1 (codeql/actions-queries, the code-scanning suite) locally on this pull request's merge commit 24a7aa6 (c8d0188 into 43a87bf). It reports 2 results, down from 3 on main: actions/missing-workflow-permissions on ci-win.yml:68 is gone, and nothing new is introduced.

Alerts #15 and #16 stay open, since this pull request does not touch nightly-test.yml (they are on lines 227 and 235 there). The query raises them only for that workflow's workflow_dispatch trigger. workflow_run runs already get read-only access to main's cache (docs). I'd handle them in a separate change: declare cache-mode: none for that workflow, since nothing in it uses the cache, then dismiss the two alerts, because the query does not model cache-mode.

Why CodeQL didn't comment here: this pull request comes from a fork, and the repository's CodeQL default setup skips fork pull requests. No CodeQL analysis ran for it, which is why I checked locally. #3848 moves to advanced setup so fork pull requests get a CodeQL check before merge. In a fork test it failed a pull request with a planted critical finding and ignored the alerts already on main.

[napetrov]

@Alexandr-Solovev I pushed two commits onto this branch (fast-forward, nothing of yours rewritten). They address my review above and close the remaining code-scanning alerts. Please check that you agree with them.

9e66d78 fix(ci): pass the real base commit to the ABI baseline lookup

• ci.yml: the base is now read from the commit object: git cat-file -p HEAD | awk '/^parent /{print $2; exit}'. In checkout's depth-1 clone, git log -1 --format=%P printed nothing, so every run took the main-tip fallback. I ran the exact new line in a reproduced --depth=1 checkout of refs/pull/3838/merge: it passes 500d5ac (the merge commit's first parent), while the old line passes an empty string.
• Empty base → ::error:: instead of the "no cached build for the base" warning. In this job an empty base can only be a caller bug.
• Any walk-back warns (the stale-after threshold is gone). A baseline that is behind the base hides a change that undoes one of the skipped commits; see the abidiff repro in my review. It happens routinely, and it happened while I was testing: main's tip 500d5ac had no cache entry because its CI run was still in_progress, so the lookup went back one commit and printed the new warning. I kept this as a warning, not an error. An error would block every pull request for roughly the 70 minutes after each merge; the warning tells the author to rerun the job.
• A failed gh cache list now reports itself and says "rerun this job". Before, it ended in "no cache on main, rerun CI on main". --repo "${GITHUB_REPOSITORY}" is passed explicitly, as the commit lookup already did. Without it, running the script from a fork clone silently lists the fork's caches.
• Provenance note corrected: workflow_run and pull_request_target runs only get read access to main's cache scope.

Script checks, run against the live repository (bash -n and shellcheck -S warning clean):

Case	Result
base cached (b78cd81)	exact: yes, 0 commits back, key= written
base's own entry not saved yet (live 500d5ac, and a stub hiding the entry)	walk-back warning, exact: no, 1 commits back
empty base	::error::No base commit was passed…, rc 1
gh cache list fails 3× (stubbed 502)	::error::Could not list the caches on refs/heads/main. Rerun this job., rc 1
unknown base SHA	::error::Could not list the commits reachable from …, rc 1

1fee65b ci(nightly-test): take cache access away with cache-mode: none

This closes the gap behind alerts #15/#16: workflow_dispatch runs of Nightly-test still get write access to main's cache scope.

• Added cache-mode: none at workflow level. cve-bin-tool-scan gets cache-mode: read, the access it has today under workflow_run, so the action can still restore its pip cache.
• The cache service enforces it, not just actions/cache. I tested this in my fork with a JavaScript action that calls CacheService/CreateCacheEntry directly using the job's runtime token. With cache-mode: none the call returns "cache write denied: token has no writable scopes"; the control job with cache-mode: write gets "ok":true.
• Removed the per-step ACTIONS_RUNTIME_TOKEN / ACTIONS_RESULTS_URL / ACTIONS_CACHE_URL blanking and the comments that require it. In the same test, a run: step on a GitHub-hosted Linux runner did not receive ACTIONS_RUNTIME_TOKEN at all; only JavaScript actions get it. So the blanking never removed anything, and it cannot take the token away from the runner process.
• GitHub accepts the key at both workflow and job level: dispatching this exact file on a branch in my fork created a run with all five jobs (skipped there by the github.repository guard). actionlint, even 1.7.12, doesn't know cache-mode yet, but this repository doesn't run actionlint.

CodeQL 2.27.1 locally on this branch reports 2 results, down from 3 on main. Alert 17 is gone; the two poisonable-step findings stay on the same two steps, because the query does not model cache-mode. The CodeQL check on this pull request (now that fork pull requests are scanned, #3848) reports "No new alerts in code changed by this pull request". After merge I will dismiss #15/#16 with a link to the note at the top of nightly-test.yml.

Description

Three statements in the description no longer match the code: the provenance paragraph ("fork pull request jobs in Nightly-test can write into main's cache scope"), "no masking either", and the "more than 5 commits behind" row of the failure-mode table. Could you update them, or should I?

ABI check result on 1fee65b: ✅ ABI Conformance(avx2) passed with __release_lnx-500d5ac… (exact: yes, 0 commits back). The baseline is the merge commit's first parent, and abidiff found nothing in the six libraries it compares. On c8d0188 the same job had logged exact: no together with the fallback warning.

[Alexandr-Solovev]

@napetrov Thanks. I reviewed both commits and agree with them. 9e66d78f7: reading the parent with cat-file is the right fix, since the job only runs on pull_request, so HEAD is always the merge commit; and I agree with warning (rather than erroring) on a walk-back. 1fee65b96: the cache-mode: none change is fine. I updated the description: the provenance paragraph, the masking claim (it now holds only for an exact match), and the failure-mode table and validation list now match the script.