fix (Backport): Clarify error when GitHub PR merge refspec fetch fails - #4255
Review submitted — approved
Review summary
Change reviewed
PR buildkite/agent#4255 backports the merged change from #4254 to the v3 branch for Linear issue A-1711. The goal is to make a failed GitHub speculative merge-ref fetch (refs/pull/N/merge) explain that the ref may be absent because of a merge conflict or because GitHub could not create it automatically.
The implementation:
- Adds
prMergeRefspecHintininternal/job/checkout_fetch.goand appends its diagnostic to failures in both canonical PR fetch variants: theCommit == "HEAD"path and the known-commit/fallback path. - Updates
internal/job/checkout_mirror.goso an existing--git-mirrors-pathmirror gets the same diagnostic. PR head refs remain retryable, while merge refs now skip the inner approximately two-minute fetch retry and fail promptly, matching the canonical fetch behavior. - Preserves the wrapped underlying error with
%w, so the checkout retry/error-classification code can still inspect the originalgitError. - Extends
TestDefaultCheckoutPhase_MergeRefspecFailsFastto assert the diagnostic and addsTestUpdateGitMirrorMergeRefspecMissingHintsMergeConflictfor the mirror-update path.
Evidence examined
I read the complete four-file diff and the full changed source and test files. I traced the checkout path through checkout, defaultCheckoutPhase, getOrUpdateMirrorDir/updateGitMirror, fetchSource, and gitFetch, including the outer checkout retry behavior and gitError wrapping. I also checked the remote-mirror eligibility path, which deliberately excludes merge-ref builds from the separate remote-mirror optimization.
I reviewed the A-1711 report and acceptance text, the backport PR description and conversation, and the merged source PR #4254. There were no existing comments or earlier reviews on #4255 to reconcile. The backport's patch payload matches #4254 exactly; the only diff-format differences were expected hunk line offsets between main and v3. The PR head remained at 3b24ddbced51078b05186260efb7a5c99aea6946 throughout the review.
git diff --check origin/v3 HEAD passed. I attempted the focused new tests, nearby checkout/mirror tests, and go vet ./internal/job, but the Go toolchain is not installed or available on PATH in this review environment, so none of those local Go checks ran. I did not run the full test suite.
At the final CI check, Socket's two checks had passed. The Buildkite agent build #14035 was still pending, and the review build was still in progress. The GitHub “Require category label” check had failed while the PR had no labels; no code-test failure was visible at review time. Direct Buildkite CLI access was unavailable for this organization, so build state was verified through the GitHub check rollup.
Findings and verdict
No actionable correctness, security, compatibility, or regression findings remained. The two supported checkout paths both receive the intended hint, PR-head retry behavior is unchanged, merge-ref fetches avoid the unnecessary inner retry, and the new tests cover the canonical and mirror-specific diagnostics.
I submitted an APPROVE review with no inline comments: #4255 (review)
Trigger source: automatic.