Fix commit verification with non-default branches

internal/job/checkout.go

@@ -299,6 +299,12 @@ func (e *Executor) checkout(ctx context.Context) error {
 			var errGit *gitError
 
 			switch {
+			case errors.Is(err, ErrCommitVerificationFailed):
+				// A commit that is provably not on its branch won't become valid by
+				// retrying, so fail fast instead of re-cloning through the whole backoff.
+				e.shell.Warningf("Checkout failed! %s", err)
+				r.Break()
+
 			case errors.Is(err, errCheckoutAttemptTimedOut):
 				// The per-attempt timeout fired and git was signal-killed.
 				// Treat this like a generic transient failure: warn, clean

internal/job/commit_verification.go

@@ -26,47 +26,77 @@ var ErrCommitVerificationUnavailable = errors.New("commit verification unavailab
 func (e *Executor) checkCommitOnBranch(ctx context.Context) error {
 	e.shell.Commentf("Verifying commit %q is on branch %q", e.Commit, e.Branch)
 
+	// The build's branch usually isn't a local ref at this point: the checkout
+	// fetches the commit directly (detached HEAD) and only the repository's
+	// default branch gets a local ref from the clone. Fetch the branch tip so
+	// there's a resolvable ref to check the commit's ancestry against, and pin it
+	// to a SHA (the deepening fetches below overwrite FETCH_HEAD). Without this,
+	// merge-base against the bare branch name fails to resolve for any non-default
+	// branch and the check silently degrades to "unavailable".
+	//
+	// Qualify the ref as refs/heads/<branch>: git resolves a bare name against
+	// refs/tags/ before refs/heads/, so a tag sharing the branch's name would pin
+	// FETCH_HEAD to the tag tip and verify the commit against the tag instead of
+	// the branch, letting a commit reachable only from the tag pass.
+	branchRef := "refs/heads/" + e.Branch
+	if fetchErr := gitFetch(ctx, gitFetchArgs{
+		Shell:      e.shell,
+		Repository: "origin",
+		RefSpecs:   []string{branchRef},
+	}); fetchErr != nil {
+		return fmt.Errorf("%w: unable to fetch branch %q: %w", ErrCommitVerificationUnavailable, e.Branch, fetchErr)
+	}
+	branchTip, err := e.shell.Command("git", "rev-parse", "FETCH_HEAD").RunAndCaptureStdout(ctx)
+	if err != nil {
+		return fmt.Errorf("%w: unable to resolve branch %q: %w", ErrCommitVerificationUnavailable, e.Branch, err)
+	}
+	branchTip = strings.TrimSpace(branchTip)
+
+	// merge-base --is-ancestor walks back from the branch tip: exit 0 means the
+	// commit is reachable from it (definitive, even on a shallow clone), exit 1
+	// means it is not. On a shallow clone a "not an ancestor" result can be a
+	// false negative when the connecting history lies beyond the shallow boundary,
+	// so a negative (or otherwise inconclusive) result is only trusted once the
+	// repository is no longer shallow; until then we deepen the branch and re-check.
 	for _, fetchFlag := range []string{"", "--deepen=50", "--unshallow"} {
 		if fetchFlag != "" {
-			// After the first iteration, try to unshallow the repo (a bit or a lot)
 			e.shell.Commentf("Deepening checkout to verify commit (%s)...", fetchFlag)
-			fetchErr := e.shell.Command("git", "fetch", fetchFlag).Run(ctx)
-			if fetchErr != nil {
+			if fetchErr := gitFetch(ctx, gitFetchArgs{
+				Shell:         e.shell,
+				GitFetchFlags: fetchFlag,
+				Repository:    "origin",
+				RefSpecs:      []string{branchRef},
+			}); fetchErr != nil {
 				return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, fetchErr)
 			}
 		}
 
-		// Try the ancestry check
-		err := e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, e.Branch).Run(ctx)
-
-		switch shell.ExitCode(err) {
-		case 0:
-			return nil // verified!
-		case 1:
-			return fmt.Errorf("%w: commit %q is not on branch %q", ErrCommitVerificationFailed, e.Commit, e.Branch)
-		case 128:
-			// unclear — continue below
-		default:
-			return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, err)
+		mergeBaseErr := e.shell.Command("git", "merge-base", "--is-ancestor", e.Commit, branchTip).Run(ctx)
+		if mergeBaseErr == nil {
+			return nil // commit is reachable from the branch tip: verified
 		}
-
-		// On the first iteration, check if the checkout is shallow. If it is
-		// not, the 128 exit code reflects some other error.
-		if fetchFlag != "" {
-			continue
+		// A non-exit error (e.g. git failed to spawn) tells us nothing about
+		// ancestry, so treat it as unavailable rather than a definitive failure.
+		if !shell.IsExitError(mergeBaseErr) {
+			return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, mergeBaseErr)
 		}
-		output, shallowErr := e.shell.Command("git", "rev-parse", "--is-shallow-repository").RunAndCaptureStdout(ctx)
+		exitCode := shell.ExitCode(mergeBaseErr)
+
+		shallow, shallowErr := e.shell.Command("git", "rev-parse", "--is-shallow-repository").RunAndCaptureStdout(ctx)
 		if shallowErr != nil {
 			return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, shallowErr)
 		}
-
-		if strings.TrimSpace(output) != "true" {
-			// Not shallow — this is a genuine error
-			return fmt.Errorf("%w: unable to verify commit %q on branch %q: %w", ErrCommitVerificationUnavailable, e.Commit, e.Branch, err)
+		if strings.TrimSpace(shallow) != "true" {
+			// Full history, so the result is now definitive.
+			if exitCode == 1 {
+				return fmt.Errorf("%w: commit %q is not on branch %q", ErrCommitVerificationFailed, e.Commit, e.Branch)
+			}
+			return fmt.Errorf("%w: unable to verify commit %q on branch %q", ErrCommitVerificationUnavailable, e.Commit, e.Branch)
 		}
+		// Still shallow, so deepen on the next iteration and re-check.
 	}
 
-	// All attempts exhausted — verification is unavailable
+	// All attempts exhausted, so verification is unavailable.
 	return fmt.Errorf("%w: unable to verify commit %q on branch %q after exhausting fetch strategies", ErrCommitVerificationUnavailable, e.Commit, e.Branch)
 }
 
@@ -96,8 +126,9 @@ func (e *Executor) verifyCommit(ctx context.Context) error {
 		return nil
 	}
 
-	// Skip if this is a PR build — the commit may be on a merge ref, not the target branch
-	if e.PullRequest != "" {
+	// Skip if this is a PR build: the commit may be on a merge ref, not the target
+	// branch. BUILDKITE_PULL_REQUEST is the string "false" (not empty) on non-PR builds.
+	if e.PullRequest != "" && e.PullRequest != "false" {
 		e.shell.Commentf("Skipping commit verification: pull request build (#%s)", e.PullRequest)
 		return nil
 	}
@@ -122,13 +153,15 @@ func (e *Executor) verifyCommit(ctx context.Context) error {
 		if e.GitCommitVerification == "strict" {
 			return err
 		}
-		e.shell.Warningf("Commit verification failed: %v", err)
+		// err already begins with "commit verification failed", so log it as-is.
+		e.shell.Warningf("%s", err)
 		return nil
 	}
 
 	// Verification unavailable — infrastructure issue, not a security concern.
 	// We always warn but never block, even in strict mode, to avoid users
 	// disabling verification entirely due to infrastructure false positives.
-	e.shell.Warningf("Commit verification unavailable: %v", err)
+	// err already begins with "commit verification unavailable", so log it as-is.
+	e.shell.Warningf("%s", err)
 	return nil
 }