Fix quicktest: stop it failing every dependabot PR, and fail with a reason instead of silently - #870
Open
m-reuter wants to merge 12 commits into
Open
Fix quicktest: stop it failing every dependabot PR, and fail with a reason instead of silently#870m-reuter wants to merge 12 commits into
m-reuter wants to merge 12 commits into
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
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.
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 gets a red quicktest check within seconds, a labelled PR from a fork spends 21
minutes building and then dies with no usable message, and a declined run reported success without
testing anything.
Setup uv and Pythonhad noif:, so it ranwhen 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.reason and exited 0, leaving the build job green with every test job skipped. They now exit 1.
pull_requestruns from a fork get no secrets, and mustnot: 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/868resolves to nothing), andrestricted 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.
github.sha. The build job and the test jobs resolved the branchor merge ref separately, twenty minutes apart, so a push in between left the tests on a different
commit than the image.
github.event.ref, absent from thepull_requestpayload, so allfour 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.
(test, reference) , and which then swapped them again internally. The values reported are
unchanged; the failure message now states that the pair is [reference, test] .
Also documents in
test/quicktest/README.mdthat the reference data is regenerated after everyrelease, which had not been recorded anywhere, and corrects two environment variables there.
Verification
All YAML parses, every
run:block passesbash -nwith the expressions stubbed, and the parsescript was exercised across the eight trigger scenarios: accepted paths exit 0 with
CONTINUE=true,and all four declined paths exit non-zero.
Caveat
The workflow pins the composite actions
@dev, so therun-fastsurferandrun-testschanges takeeffect only once merged.
fail-fastis deliberately left on.