Fix commit verification with non-default branches#4094
Draft
petetomasik wants to merge 5 commits into
Draft
Conversation
checkCommitOnBranch ran `git merge-base --is-ancestor <commit> <branch>` against the bare BUILDKITE_BRANCH name, but at verification time only the repository's default branch exists as a local ref. Git does not resolve a bare name to refs/remotes/origin/*, so merge-base exited 128 for any non-default branch and the check degraded to "unavailable" (warn-only, even under strict). Strict verification therefore only ever enforced on the default branch. Fetch the branch tip and pin it to a SHA, then check ancestry against that SHA. Only trust a "not an ancestor" result once the repo is non-shallow, and treat a merge-base that fails to run as unavailable rather than a definitive failure. Fast-break the checkout retry loop on a definitive failure so a provably-off-branch commit fails immediately instead of re-cloning through the full backoff.
Cover the paths the non-default-branch fix introduced: a shallow clone with a genuinely off-branch commit must deepen and then block, the retry loop must fast-break rather than re-verify on every attempt, and an unavailable check must stay warn-only even under strict. Strengthen the warn-mode test to prove it swallows a genuine failure rather than passing because the check silently degraded. Add a regression test for the original bug: a build on a non-default branch with an off-branch commit.
verifyCommit skipped verification whenever PullRequest was non-empty, but BUILDKITE_PULL_REQUEST is the string "false" on every ordinary non-PR build. The guard therefore fired on almost all builds and returned before the ancestry check ran, logging "Skipping commit verification: pull request build (#false)". Skip only for real PR builds, matching how the rest of the checkout treats the "false" sentinel.
The warn-mode branches logged "Commit verification failed/unavailable:" in front of an error already wrapped with the matching sentinel, so the message read "Commit verification failed: commit verification failed: ...". Log the error directly, since it already carries the prefix.
The two shallow-clone subtests fetched over githttptest, which serves via `git upload-pack --stateless-rpc`. Shallow deepening over that transport is unreliable across git versions: on CI, `git fetch --deepen` marked the repo non-shallow without fetching the deep ancestor, so "passes after deepening a shallow clone" saw the commit as unavailable and failed. The production code is unaffected (real remotes deepen correctly). Build the fixtures in a bare repo served over file://, where shallow deepening is deterministic, via a shared setupFileBackedRepo helper. The ancestry relationships are genuine, so the verdict is correct regardless of shallow-negotiation quirks.
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
1 participant
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.
Description
checkout.commit_verification(BUILDKITE_GIT_COMMIT_VERIFICATION) verifies that a build's commit is an ancestor of the build's branch before checkout, catching a build pointed at a commit that isn't actually on the claimed branch. Understrictit should fail the build; underwarnit logs and continues.The check only ever enforced on a repository's default branch.
checkCommitOnBranchrangit merge-base --is-ancestor <commit> <branch>against the bareBUILDKITE_BRANCHname, but at verification time only the default branch exists as a local ref (the clone checks it out; the build's own branch is fetched as a detached commit, not a local branch ref). Git does not resolve a bare name likefeaturetorefs/remotes/origin/feature, so merge-base exited 128 for any non-default branch. That 128 was classified as "unavailable", which is warn-only even understrict, so a build on a non-default branch with a commit genuinely not on that branch ran to completion.The fix fetches the branch tip, pins it to a SHA, and checks ancestry against that SHA instead of the unresolvable bare name. Shallow clones are handled by trusting a "not an ancestor" result only once the repo is non-shallow (a shallow clone can report a genuine ancestor as "not an ancestor" when the connecting history is beyond the shallow boundary), deepening as needed. A merge-base that fails to run at all is treated as "unavailable" rather than a definitive failure. The checkout retry loop now fast-breaks on a definitive failure so a provably-off-branch commit fails immediately instead of re-cloning through the full backoff.
A second bug disabled verification on nearly every build:
verifyCommitskipped wheneverBUILDKITE_PULL_REQUESTwas non-empty, but the agent sets that variable to the string"false"(not empty) on every non-PR build. The check therefore returned early before ever running. It now skips only for real pull-request builds, matching how the rest of the checkout treats the"false"sentinel.Alternative considered: resolving the branch to
refs/remotes/origin/<branch>rather than fetching. Rejected because the remote-tracking ref isn't reliably populated in the commit-specified checkout path (only the commit is fetched), so fetching the branch tip is what guarantees a resolvable target.One behavior was left deliberately unchanged: if fetching the branch fails (branch deleted/renamed on the remote, network blip), the check reports "unavailable" and does not block, even under
strict. This matches the existing documented design that infrastructure failures never block verification, so users don't disable it over false positives.Context
Linear. Found while testing
checkout.commit_verification: strictin a pipeline: a build created from a non-default branch with a commit SHA not on that branch allowed the jobs to complete instead of failing.Changes
internal/job/commit_verification.go: rewritecheckCommitOnBranchto fetch and pin the branch tip, check ancestry against the SHA, trust "not an ancestor" only when the repo is non-shallow, and treat a non-exit merge-base error as unavailable.internal/job/commit_verification.go: skip verification only for real pull-request builds (BUILDKITE_PULL_REQUESTis the string"false", not empty, on ordinary builds), and drop a duplicated prefix from the warn-mode log messages.internal/job/checkout.go: fast-break the checkout retry loop onErrCommitVerificationFailedso a provably-off-branch commit fails immediately.strict, a strengthened warn-mode assertion, a regression test for a build on a non-default branch, and a non-PR build (BUILDKITE_PULL_REQUEST=false) that must still verify.file://, because shallow deepening over the HTTP test server's stateless-rpc transport is not reliable across git versions.No CLI argument changes.
Testing
go test ./...)go tool gofumpt -extra -w .)Disclosures / Credits
I used Claude Code (Opus 4.8) to diagnose the bug, implement the fix and tests, run a structured code review, and address the findings, all under my direction and review. I made the design decisions (including keeping the "unavailable never blocks" behavior under
strict) and verified the changes fixed the issue with non-default branches.