Fix commit verification with non-default branches#4094
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.
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one blocking edge case in the new branch-tip fetch: ambiguous branch/tag names can make verification check a tag instead of the branch.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 6006, then answer my questions about the findings.
An unqualified 'git fetch origin <branch>' resolves refs/tags/ before refs/heads/, so a tag sharing the build branch's name pins FETCH_HEAD to the tag tip and merge-base then verifies the commit against the tag. A commit reachable only from the tag would pass verification even when it is not on the branch. Fetch refs/heads/<branch> via the gitFetch helper at both the initial and deepening sites so the branch ref always wins.
Previous concerns have been addressed.
There was a problem hiding this comment.
The previous branch/tag collision concern is addressed, and I didn't find new issues in this pass. I'm leaving this as a comment rather than an approval because this touches the agent checkout/commit-verification path, which is above my approval ceiling; the Buildkite agent check was still pending when I reviewed.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 6018, then answer my questions about the findings.
checkCommitOnBranch fetched the branch tip via gitFetch, which runs shellwords.Split on every refspec. BUILDKITE_BRANCH is externally controlled and quotes are legal in git refs, so a branch name like john's-branch made the split error out (verification degraded to "unavailable", which never blocks even under strict) while balanced quotes were stripped and fetched a different ref. Either way an off-branch commit could run under strict verification. Fetch the branch tip with a discrete argument vector instead, so the ref reaches git intact. For legal branch names the git invocation is identical to the previous gitFetch call.
The branch-tip fetch is the one network dependency the ancestry check adds. A single transient failure degraded the whole check to "unavailable", which warns but never blocks even under strict, and the outer checkout retry loop does not re-run a verification that returned nil, so one blip permanently skipped verification for that build. Wrap the fetch in a small bounded retrier so a transient failure gets a few attempts before the check gives up.
The non-default-branch coverage only exercised the failure direction. Add the happy-path companion: a valid commit on a non-default branch must verify and let the build proceed, rather than silently degrading to "unavailable". This guards the branch-tip fetch end-to-end.
setupFileBackedRepo and setupTagBranchCollisionRepo duplicated the env setup, bare-repo init, working clone, and commit helper. Pull that into newFileBackedRepo so each fixture only builds its own branch/tag topology.
Add two subtests: one where the target commit sits beyond the --deepen=50 boundary so only --unshallow can reach it, and one proving a non-PR build (BUILDKITE_PULL_REQUEST=false) still runs verification rather than being skipped by the "false" sentinel.
There was a problem hiding this comment.
The new file-backed fixture needs a Windows-safe URL before its tests can pass the repository's Windows matrix.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 6803, then answer my questions about the findings.
Co-authored-by: buildsworth-bk-app[bot] <265670805+buildsworth-bk-app[bot]@users.noreply.github.com>
Previous concerns have been addressed.
There was a problem hiding this comment.
The Windows-safe file URL addresses the previous concern, but the added block now fails the repository's gofumpt check.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 6805, then answer my questions about the findings.
Co-authored-by: buildsworth-bk-app[bot] <265670805+buildsworth-bk-app[bot]@users.noreply.github.com>
Previous concerns have been addressed.
There was a problem hiding this comment.
The formatting concern is fixed. I found one new blocking gap: the verification fetches bypass the agent’s configured fetch flags, which can make strict verification silently unavailable.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 6807, then answer my questions about the findings.
The branch-tip and deepening fetches in checkCommitOnBranch ran a bare git fetch, dropping the operator's configured git-fetch flags. A repo that needs a flag such as --upload-pack to reach its server completes the checkout's clone and source fetch but fails here, and verifyCommit classifies that failure as unavailable, letting strict mode continue. Split the configured flags and thread them through both fetches while keeping the branch ref a single unsplit argument.
…branch-commit-verification
Previous concerns have been addressed.
There was a problem hiding this comment.
The previous fetch-flags concern is addressed. I found a new blocking interaction between configured depth flags and the explicit deepening fetches.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 7176, then answer my questions about the findings.
git rejects --depth combined with --deepen or --unshallow, so a configured BUILDKITE_GIT_FETCH_FLAGS=--depth=1 made the deepening fetches exit 128. That degraded a genuinely off-branch commit on a shallow clone to "unavailable" (warn-only, even under strict), reintroducing the vulnerability the check exists to catch. Keep the transport flags on the deepening fetches but drop the depth-limiting ones. Also correct a stale integration mock expectation that predated fetch-flag preservation.
Previous concerns have been addressed.
There was a problem hiding this comment.
The previous depth-flag interaction is fixed. I found one new blocking FETCH_HEAD pinning issue in the configured-fetch-flags path.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 7180, then answer my questions about the findings.
With the operator's fetch flags preserved, a configured --append, --no-write-fetch-head or --dry-run leaves FETCH_HEAD pointing at an earlier fetch (the build commit recorded by fetchSource). rev-parse FETCH_HEAD then resolves to the build commit itself, so merge-base <commit> <commit> trivially succeeds and strict verification accepts an off-branch commit. Fetch the branch tip through an explicit-destination refspec into a dedicated local ref and resolve that ref instead. The ref is written regardless of those flags and is deleted again on return. A blocklist of FETCH_HEAD-affecting flags would be fragile; the dedicated ref is robust by construction.
Previous concerns have been addressed.
There was a problem hiding this comment.
The dedicated ref addresses the previous FETCH_HEAD concern. I found one new blocking case where configured fetch modes can still leave that ref unpinned.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 7181, then answer my questions about the findings.
The dedicated branch-tip ref is still not guaranteed to be written when the operator's fetch flags include a mode that skips or redirects the ref update: --dry-run fetches nothing, --prefetch rewrites the destination under refs/prefetch/, and --negotiate-only fetches no packfile. rev-parse of the ref then fails, the check degrades to "unavailable", and strict treats that as a pass, so a reused checkout whose off-branch commit is already local proceeds unverified. Drop those modes from every verification fetch. -n is git fetch's --no-tags (not --dry-run), so it is left intact.
There was a problem hiding this comment.
The previous fetch-mode concern remains open; I added the remaining edge case in its thread. I didn't find a separate new issue in this pass.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 7182, then answer my questions about the findings.
git accepts unambiguous abbreviations such as --dry and --prefe, so the exact-match filter left those active: the branch-tip ref stayed unwritten and strict verification could still continue in a reused checkout. Drop any --flag whose name is a prefix of dry-run, prefetch or negotiate-only. Stripping tokens keeps this version-safe. Appending --no-* forms would break older git: --prefetch (2.29) and --negotiate-only (2.32) postdate the oldest git the agent supports, so --no-prefetch/--no-negotiate-only would fail there and degrade verification to a pass. --prune, --negotiation-tip and --no-* negations are not prefixes of these names, so they survive.
Previous concerns have been addressed.
There was a problem hiding this comment.
The previous fetch-mode concern is fixed. I found a new blocking abbreviation case in the existing configured-depth thread.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 7183, then answer my questions about the findings.
stripShallowFetchFlags matched exact names, so git's accepted abbreviations slipped through: --dept=1 is --depth=1 but survived into the deepening flags, making the --deepen=50 fetch exit 128 and degrade strict verification to unavailable (a pass) on a shallow clone. Drop any --flag whose name is a prefix of depth, deepen, shallow-since, shallow-exclude or unshallow, still skipping the space-form value token. This mirrors the same fix already applied to the ref-suppressing flags.
Previous concerns have been addressed.
There was a problem hiding this comment.
The abbreviation case is fixed. I found one related blocking --unshallow interaction in the existing configured-depth thread.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 7187, then answer my questions about the findings.
stripShallowFetchFlags was only applied to the deepening fetches, so a
configured --unshallow (or its --unsh abbreviation) still reached the
initial branch-tip fetch. Once fetchSource has completed the clone,
git fetch --unshallow exits 128 ("--unshallow on a complete repository
does not make sense"), which degrades strict verification to unavailable
(a pass) and lets an off-branch commit through.
Strip the depth-limiting flags from the flags shared by all verification
fetches. Reshaping history is the deepening loop's job, so the operator's
shallow flags have no place in any of these fetches.
Previous concerns have been addressed.
There was a problem hiding this comment.
The previous --unshallow concern is fixed, and I didn't find a new issue in this pass. I'm leaving this as a comment because this changes the agent's commit-verification path, which is above my approval ceiling.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 7193, then answer my questions about the findings.
2 participants
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. The branch is fetched as
refs/heads/<branch>so a tag sharing the branch's name can't shadow it, using a directgit fetchrather than the shared fetch helper (which word-splits its refspecs and would corrupt a branch name containing shell metacharacters that are legal in git refs). The fetch is retried a few times before giving up, so a transient failure doesn't silently skip verification. 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.One behavior was left deliberately unchanged: if fetching the branch keeps failing (branch deleted or renamed on the remote, or the network stays down through the retries), 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 (asrefs/heads/<branch>, via a directgit fetchso the ref isn't word-split, and with a bounded retry), 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, a valid commit on a non-default branch that must pass, a non-PR build (BUILDKITE_PULL_REQUEST=false) that must still verify, a commit reachable only after--unshallow, a transient branch-fetch failure that must retry rather than degrade, and a branch name containing shell metacharacters.file://-backed cases share a fixture helper and use a bare repo served overfile://, 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.