fix(auto-merge): match required checks by normalised name, and require all of them - #39
Merged
Merged
Conversation
…e all of them Three repos name their OSV job osv-scanner, which endswith() matched against neither accepted literal, so they could not adopt this workflow at all -- it would refuse every dependabot PR whose OSV check had reported and passed. The header asserted required-check names are fleet-invariant; measured across the four repos carrying it, they are not, two spellings split 3-1. Adding a third literal would fix the instance and leave the class, so both sides are now stripped to alphanumerics and tested for containment. A second defect had to go with it: the old code took [0] of the matching checks, so a token selecting more than one made the verdict depend on the order GitHub returned them, and a superseded run could answer for a live one. Looser matching would have made that worse. Every matching check must now pass, which for a guard whose failure mode is auto-merging is the only defensible direction. Verified against a fixture of the real check names from all four repos: the osv token matches all three spellings, every other token matches exactly its own check, PII Scan stays clear, and both refusal paths (a red check, an absent check) set failed=1.
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.
Finding
The reusable auto-merge matched required checks with
endswith()against literal spellings. Threefleet repos name their OSV job
osv-scanner, which ends with neither accepted leaf —"osv-scanner"does not end with
"osv scanner", and it ends with"scanner", not"osv-scan". Those repos couldnot adopt this workflow at all: it would refuse every dependabot PR whose OSV check had reported and
passed.
Evidence
dependabot-auto-merge.yml:63—require_passed_check "osv scanner" "osv-scan"akroasis,epistole,hamma—security.ymljob namedosv-scannerkanon—security.ymljob namedosv scanner(the one that matched)The file's own header asserted the premise this contradicts: "Required-check names are
fleet-invariant because security.yml + gate-attestation.yml are canonical." Measured across the four
repos carrying it, they are not — two spellings, split 3–1, hyphen in the majority.
Reported by @t0-thumos as #38 while sweeping the
exit 0auto-merge-guard class.Why this matters
It fails closed, so nothing merged that should not have. The cost is the migration path: the
repos still carrying a local copy of this workflow are exactly the ones whose local copy has the
exit 0defect, and this was the blocker stopping them from replacing it with the fixed shared one.Adding
osv-scanneras a third literal would fix the instance and leave the class. An invarianceasserted in a comment and enforced nowhere has already drifted once; the matcher is what should
tolerate it.
Desired correction
Strip every non-alphanumeric from both the reported check name and the token, then test containment.
osv scanner,osv-scanner, andosv scanner / osv-scanall normalise to something containingosvscan, so the whole spelling class matches without enumerating it.A second defect fixed alongside, because looser matching would otherwise have made it worse. The
old code took
[0]of the matching checks, so when a token selected more than one, the verdictdepended on the order GitHub returned them — a superseded run could answer for a live one. Every
matching check must now pass. For a guard whose failure mode is auto-merging, that is the only
defensible direction.
Verified by running the matcher against a fixture of the real check names from all four repos:
osvscanosv scanner,osv-scanner,osv scanner / osv-scan— all threecargodenycargo denycargoauditcargo auditgateattestationgate-attestationgategategate / gateUnrelated checks stay clear:
PII Scannormalises topiiscan, which containsscanbut notosvscan. Both refusal paths were exercised too — a red OSV check and a wholly absent check eachset
failed=1.Done when:a repo naming its OSV jobosv-scannercan adopt this workflow and have its dependabotPRs auto-merge on green.
Closes #38