fix(auto-merge): wait for the verification checks to appear, not just to finish - #47
Merged
Merged
Conversation
… to finish `gh pr checks --watch` waits only on the checks that exist WHEN WATCHING STARTS. On a fresh dependabot PR the fast checks report first, so `--watch --required` returned as soon as `cargo audit` and `cargo deny` passed while `gate / gate` was still building. Verification then ran against a check list that did not contain the gate yet, took its "no matching check" branch, and refused a PR whose gate went on to pass green on the same head. The irony is the whole reason this file is preferred over branch protection: its distinguishing feature is noticing when a real verification check fails to report AT ALL, and it evaluated that at the one moment a slow check legitimately has not reported. Gate Attestation trips it everywhere, because compiling the workspace makes it reliably the slowest — the check most worth waiting for is the one most likely to be missed. Now it polls until every token group is present AND terminal, then judges. Two properties are load-bearing: - The timeout REFUSES rather than falling through. A group that never reports is exactly the condition this guard exists to catch, so exhausting the wait must refuse; falling through would reintroduce the bug in the unsafe direction. - One `gh pr checks` fetch per poll, reused across all four groups. The previous version fetched once PER TOKEN, so the groups were answered from four snapshots taken seconds apart — a second instance of the same race, and no single consistent view of the PR was ever evaluated. `tests/dependabot-auto-merge.sh` drives the step against synthetic snapshots, including the case the fix exists for: a gate absent on the first poll, pending on the second, green on the third. It earned its place before it was committed. The first version of this fix named its array `GROUPS` — which bash maintains itself as the current user's supplementary group IDs. The assignment was silently ignored, the loop iterated numeric GIDs, and every PR was refused. The YAML parsed, the shell raised nothing, and reading it showed a correct-looking array; only running it showed GIDs. The array is `CHECK_GROUPS` now, with that recorded at the site. The harness also found that a hardcoded 30s poll made the step untestable at speed, so the interval takes an env override that nothing in CI sets. Closes #46
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.
Closes #46.
Finding
gh pr checks --watchwaits only on the checks that exist when watching starts. On a freshdependabot PR the fast checks report first, so
--watch --requiredreturned as soon ascargo auditand
cargo denypassed whilegate / gatewas still building. The reported-checks verification thenran against a list that did not contain the gate yet, took its
No required verification check matching any of: gateattestation gategatebranch, and refused a PRwhose gate went on to pass green on the same head.
@t0-akroasisdiagnosed this from akroasis #454/#455 and filed it with the run log and thenormalised-token table.
The irony is the whole reason this file is preferred over branch protection. Its distinguishing
feature is noticing when a real verification check fails to report at all — and it evaluated that at
the one moment a slow check legitimately has not reported. Gate Attestation trips it everywhere,
because compiling the workspace makes it reliably the slowest. The check most worth waiting for is the
one most likely to be missed.
What changes
Poll until every token group is present and terminal, then judge. Two properties are load-bearing
and both are commented at the site:
condition this guard exists to catch, so exhausting the wait must refuse. Falling through would
reintroduce the bug in the unsafe direction — auto-merging on an absent verification.
gh pr checksfetch per poll, shared across all four groups. The previous code fetched onceper token, so the four groups were answered from four snapshots taken seconds apart. That is a
second instance of the same race, and it means no single consistent view of the PR was ever
evaluated.
Matching, normalisation, and the every-match-must-pass rule are unchanged.
Verification
tests/dependabot-auto-merge.shextracts the step's shell and drives it against synthetic checksnapshots:
The last case is the one that matters — absent on poll 1, pending on poll 2, green on poll 3, and it
accepts. The old code refused at poll 1.
The harness earned its place before it was committed
The first version of this fix named its array
GROUPS. Bash maintainsGROUPSitself as thecurrent user's supplementary group IDs; the assignment was silently ignored, the loop iterated
numeric GIDs, matched no check, and refused every PR — the same always-refuse symptom being fixed
here, reached a different way.
The YAML parsed. The shell raised nothing. Reading it showed a correct-looking array. Only running it
showed
1000 (MISSING),18 (MISSING),39 (MISSING). That is recorded as a WARNING at thedeclaration so the name cannot drift back.
The harness then found a second thing: a hardcoded 30-second poll made the step impossible to exercise
at test speed, so the late-arriving-gate case could not reach its third snapshot and reported a false
refusal. The interval now takes an env override that nothing in CI sets. A step that cannot be run
faster than production cannot be tested, and this one is shell inside YAML that nothing else checks.
shellcheck -s bash tests/dependabot-auto-merge.shexits 0.Scope
Adopting repos need no change — the reusable's interface is unchanged. Repos that adopted it and saw
auto-merge refuse green PRs (akroasis #454, #455 were merged by hand) should work from the next
dependabot PR without intervention.