Fix quicktest: stop it failing every dependabot PR, and add fixes - #870
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Fork testing exposes secrets to untrusted code, and mutable PR refs can mismatch built and tested commits.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves quicktest gating, diagnostics, and manual PR testing.
Changes:
- Skips irrelevant label runs and rejects unsupported fork runs early.
- Adds PR-ref checkout propagation and concurrency grouping.
- Validates secrets and fails HTTP downloads clearly.
File summaries
| File | Description |
|---|---|
.github/workflows/quicktest.yaml |
Updates gating, PR refs, and concurrency. |
.github/actions/run-tests/action.yml |
Propagates checkout refs and validates downloads. |
.github/actions/run-fastsurfer/action.yml |
Safely handles secrets and download failures. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Fork requests currently appear successful without running tests, and the PR description contradicts the implemented SHA and fork behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The concurrency grouping still allows unrelated label events to cancel an active quicktest run.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
m-reuter
force-pushed
the
quicktest
branch
2 times, most recently
from
September 5, 2026 14:32
3b9b8e3 to
933eb96
Compare
The uv setup step ignored the workflow's own decision to skip, so its post step failed the job, and that is the only step every sampled dependabot run fails at. The decision now happens at job level. Also fixes the concurrency group, which keyed on a field the pull_request payload does not have, docker-image=pr/<no>, which resolved to no valid ref, and fork pull requests, which are now declined with a reason instead of building for twenty minutes first.
curl had no --fail, so a missing secret or a dead url stored the error page as the T1 or the reference archive and broke somewhere unrelated later. The urls and the license are now checked first, the license goes through the environment rather than the script text, and run-tests checks out the ref it is given instead of the default branch.
Nothing recorded the convention, so it gets missed and the resulting drift reads as regressions. Also corrects the environment variables: the documented SUBJECTS_LIST does not exist and its line was cut off mid-sentence, while MAX_SUBJECTS was undocumented.
refs/pull/<no>/head moves, and the build job and the test jobs resolved it separately, so a push during the twenty-minute build left the tests running a different commit than the image. The head sha is now resolved once and passed on, and a docker-image that is not pr/<number> fails with a message instead of at checkout.
Resolving pr/<no> made a manual dispatch build and run fork code in the job that mounts the license, which no approval gate would fix, since the fork's code runs with the license either way. The head repository is now read from the same API call as the head commit and the dispatch is refused unless the pull request belongs to this repository. The three messages that recommended the dispatch to fork authors now say to merge the branch into a branch of this repository.
Restricted to this repository, pr/<no> only did what dispatching on the branch already does, and its one distinct capability was running a fork's code in the job that holds the FreeSurfer license. Removing it takes the fork guard with it. Every checkout now takes github.sha instead of the branch or the merge ref, which the build job and the test jobs resolved separately twenty minutes apart, so a push in between left the tests on a different commit than the image.
…judging them Restricted to this repository, pr/<no> only did what dispatching on the branch already does, and its one distinct capability was running a fork's code in the job that holds the FreeSurfer license. A fork pull request is now decided in the job condition, which GitHub reports as skipped; it used to print a reason and exit 0, so the workflow passed without having tested anything. Checkouts take github.sha, which the build and test jobs previously resolved separately twenty minutes apart.
The group is evaluated when the run is created, before the job condition, so with every label of a pull request in one group, attaching an unrelated label cancelled a quicktest that was already running. Re-applying quicktest still replaces its own run.
The tar of the subject folder was written beside the folder while upload-artifact was given the folder itself, so it was gzipped and then ignored. load-processed now takes the processing from the runner instead of fetching its own artifact back, and run-tests no longer checks out a second copy of the repository the job already has. Clean up author/created contributions visible in git history.
… of the tag A build of the tag resolves dependencies again and can pick up newer ones than the release shipped, so the two do not always agree. Also records which variant and settings to use: the runner has no GPU, so the CPU image rather than latest, at the runner's core count.
The direction cosines are computed rather than snapped below 1mm, so Mdc carries about 8.8e-18 of dust and every one of the 39 shared 0.8mm files failed the header check forever. Float fields are now compared at rtol=1e-6, atol=1e-6 and everything else stays exact, which still reports the dtype and fov differences in that same pair.
m-reuter
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every dependabot PR got a red quicktest check within seconds, a labelled PR from a fork spent 21 minutes building and then died with no usable message, a declined run reported success without testing anything, and the runs that did complete compared headers in a way that could never pass below 1 mm.
The workflow
Dependabot red X. The label gate worked, but Setup uv and Python had no if: , so it ran when the workflow had decided to do nothing and its post step failed the job. Across 8 dependabot runs spanning four months that is the only step any of them fails at. The label decision moves to a job-level if: , so no runner starts.
A declined run no longer looks green. Both refusals, fork PR and non-collaborator, printed a reason and exited 0, leaving the build job green with every test job skipped. They now exit 1.
Fork PRs are refused, not enabled. pull_request runs from a fork get no secrets, and must not: the PR's code is built and run in the job that mounts the FreeSurfer licence, and no approval gate changes that. Test a fork contribution by merging its branch into a branch of this repository.
docker-image=pr/<no>is removed. It never worked ( ref: pr/868 resolves to nothing), and restricted to this repository it only did what dispatching on the branch already does. Its one distinct capability was running a fork's code with the licence.Every checkout is pinned to
github.sha. The build job and the test jobs resolved the branch or merge ref separately, twenty minutes apart, so a push in between left the tests on a different commit than the image.Concurrency group keyed on
github.event.ref, absent from the pull_request payload, so all four labels dependabot attaches shared one group and cancelled each other: 78 of 113 PR runs are cancelled.-
curlhad no--fail, so a dead URL stored the error page as the T1 or the reference archive. The URLs and the licence are checked for emptiness first, and the licence now reaches the file via the environment rather than being interpolated into the script.The subject directory
The tar of the subject folder was written beside the folder while upload-artifact was given the folder itself, so the archive was gzipped and then ignored. load-processed now takes the processing from the runner instead of fetching its own artifact back, and run-tests no longer checks out a second copy of the repository the job already has, which is also why it is renamed to
Run quicktest: it never ran FastSurfer.The header comparison
Float fields are compared with a tolerance. assert_same_headers delegated to nibabel's
get_headers_diff, which compares exactly and takes no tolerance. Below 1 mm the direction cosines are computed rather than snapped, so Mdc carries float dust that no rerun reproduces, and every non-1 mm file failed the header check permanently, whatever the reference contained. Measured across the sharedmri/*.mgzof both quicktest subjects against a released cpu-v2.5.4 run:The 0.8mm subject goes from 0 of 39 files passing the header check to 29, and the 10 that still fail do so for the two real reasons. Float fields are relaxed at
rtol=1e-6, atol=1e-6; anything that is not a float array of matching shape keeps the exact comparison, which is what keeps type and the 256 mm fov gap visible while swallowing 1e-18. (The fov differences are the separate bug fixed in #873, and clear once that is released and the reference regenerated.)The failure message says which header is which. The parameters were named (expected, actual) but bound to (test, reference) , and then swapped again internally. The values reported are unchanged; the message now states that the pair is [reference, test] .
Documentation
test/quicktest/README.mdnow records that the reference data is regenerated after every release, which had not been written down anywhere, and that it must come from the released container image rather than a build of the tag, since a build of the tag resolves dependencies again and can pick up newer ones than the release shipped. It also names the variant and settings to use, corrects two environment variables, and the workflow links point at this repository instead of github.com.Verification
All YAML parses, every run: block passes bash -n with the expressions stubbed, and the parse script was exercised across the eight trigger scenarios: accepted paths exit 0 with CONTINUE=true , and all four declined paths exit non-zero.
The header change adds
test/image/test_quicktest_header_tolerance.py, 9 tests covering the dust that must pass, a real orientation difference, afovgap and adtypechange that must all still fail, and the eligibility rule itself. They live intest/imagebecausetest/quicktest/conftest.pyrequiresREF_DIRandSUBJECTS_DIRat import, so nothing in that directory is collectable without a reference dataset, while test/image is a matrix entry inunittest.yamland runs on every push. 413 image tests pass, ruff clean.Caveat
The workflow pins the composite actions @dev , so the run-fastsurfer and run-tests changes take effect only once merged. fail-fast is deliberately left on.