diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index 99a45e4652..3f8f955ba7 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -59,6 +59,7 @@ type BootstrapConfig struct { Plugins string `cli:"plugins"` Secrets string `cli:"secrets"` PullRequest string `cli:"pullrequest"` + PullRequestHeadCommit string `cli:"pull-request-head-commit"` PullRequestUsingMergeRefspec bool `cli:"pull-request-using-merge-refspec"` GitSubmodules bool `cli:"git-submodules"` GitLFSEnabled bool `cli:"git-lfs-enabled"` @@ -198,6 +199,12 @@ var BootstrapCommand = cli.Command{ Usage: "The number/id of the pull request this commit belonged to", EnvVar: "BUILDKITE_PULL_REQUEST", }, + cli.StringFlag{ + Name: "pull-request-head-commit", + Value: "", + Usage: "The expected head commit for a pull request build", + EnvVar: "BUILDKITE_PULL_REQUEST_HEAD_COMMIT", + }, cli.BoolFlag{ Name: "pull-request-using-merge-refspec", Usage: "Whether the agent should attempt to checkout the pull request commit using the merge refspec. This feature is in private preview and requires backend enablement—contact support to enable (default: false)", @@ -525,6 +532,7 @@ var BootstrapCommand = cli.Command{ PluginsAlwaysCloneFresh: cfg.PluginsAlwaysCloneFresh, PluginsPath: cfg.PluginsPath, PullRequest: cfg.PullRequest, + PullRequestHeadCommit: cfg.PullRequestHeadCommit, PullRequestUsingMergeRefspec: cfg.PullRequestUsingMergeRefspec, Queue: cfg.Queue, RedactedVars: cfg.RedactedVars, diff --git a/internal/job/checkout_fetch.go b/internal/job/checkout_fetch.go index fc93c7f297..cb5bfb727d 100644 --- a/internal/job/checkout_fetch.go +++ b/internal/job/checkout_fetch.go @@ -147,6 +147,11 @@ func (e *Executor) fetchSource(ctx context.Context, addBloblessFilter bool, atte }); err != nil { return fmt.Errorf("fetching PR refspec %q: %w", refspecs, err) } + if kind == refspecGithubPRMerge && e.PullRequestHeadCommit != "" { + if err := e.validateGithubPRMergeHead(ctx); err != nil { + return err + } + } } else { // The build is pinned to an immutable commit, and the canonical // refs/pull/* fetch exists only to obtain its objects, so a @@ -205,6 +210,52 @@ func (e *Executor) fetchSource(ctx context.Context, addBloblessFilter bool, atte return nil } +func (e *Executor) validateGithubPRMergeHead(ctx context.Context) error { + commit, err := e.shell.Command("git", "cat-file", "commit", "FETCH_HEAD").RunAndCaptureStdout( + ctx, + shell.ShowStderr(false), + ) + if err != nil { + return &gitError{ + error: fmt.Errorf("verifying fetched GitHub pull request merge commit has expected head %q: %w", e.PullRequestHeadCommit, err), + Type: gitErrorFetch, + } + } + + actualHead, ok := commitSecondParent(commit) + if !ok { + return &gitError{ + error: fmt.Errorf("verifying fetched GitHub pull request merge commit has expected head %q: fetched commit has fewer than two parents", e.PullRequestHeadCommit), + Type: gitErrorFetch, + } + } + + if actualHead != e.PullRequestHeadCommit { + return &gitError{ + error: fmt.Errorf("fetched GitHub pull request merge commit does not match the build's pull request head: expected %q, got %q", e.PullRequestHeadCommit, actualHead), + Type: gitErrorFetch, + } + } + + return nil +} + +func commitSecondParent(commit string) (string, bool) { + parents := 0 + for _, line := range strings.Split(commit, "\n") { + if line == "" { + break + } + if parent, ok := strings.CutPrefix(line, "parent "); ok { + parents++ + if parents == 2 { + return parent, true + } + } + } + return "", false +} + func isExistingCheckoutRemoteMirrorAttempt(attempt *remoteMirrorAttempt) bool { return attempt != nil && attempt.site == remoteMirrorSiteExistingCheckout && diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index 06546e513a..5c8a774296 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -125,11 +125,15 @@ func TestDefaultCheckoutPhase(t *testing.T) { executor: &Executor{ shell: shell, ExecutorConfig: ExecutorConfig{ - Commit: "HEAD", - Branch: "main", - CleanCheckout: false, - GitCleanFlags: "-f -d -x", - RefSpec: "refs/custom", + Commit: "HEAD", + Branch: "main", + CleanCheckout: false, + GitCleanFlags: "-f -d -x", + RefSpec: "refs/custom", + PullRequest: "124", + PipelineProvider: "github", + PullRequestHeadCommit: "not-the-head", + PullRequestUsingMergeRefspec: true, }, }, projectName: "project-name-refspec", @@ -140,12 +144,13 @@ func TestDefaultCheckoutPhase(t *testing.T) { executor: &Executor{ shell: shell, ExecutorConfig: ExecutorConfig{ - PullRequest: "124", - Commit: "HEAD", - Branch: "main", - CleanCheckout: false, - GitCleanFlags: "-f -d -x", - PipelineProvider: "github", + PullRequest: "124", + PullRequestHeadCommit: "not-the-head", + Commit: "HEAD", + Branch: "main", + CleanCheckout: false, + GitCleanFlags: "-f -d -x", + PipelineProvider: "github", }, }, projectName: "project-name-pull-request", @@ -194,6 +199,7 @@ func TestDefaultCheckoutPhase(t *testing.T) { CleanCheckout: false, GitCleanFlags: "-f -d -x", PipelineProvider: "github", + PullRequestHeadCommit: "not-the-head", PullRequestUsingMergeRefspec: true, }, }, @@ -224,6 +230,35 @@ func TestDefaultCheckoutPhase(t *testing.T) { } } +func TestCommitSecondParent(t *testing.T) { + t.Parallel() + + for _, test := range []struct { + name string + commit string + want string + ok bool + }{ + { + name: "merge commit", + commit: "tree tree-id\nparent base-id\nparent head-id\nauthor Example\n\nMessage\n", + want: "head-id", + ok: true, + }, + { + name: "non-merge commit", + commit: "tree tree-id\nparent base-id\nauthor Example\n\nparent fake-head-in-message\n", + }, + } { + t.Run(test.name, func(t *testing.T) { + got, ok := commitSecondParent(test.commit) + if got != test.want || ok != test.ok { + t.Errorf("commitSecondParent() = (%q, %t), want (%q, %t)", got, ok, test.want, test.ok) + } + }) + } +} + func TestPrepareGitSSHKey(t *testing.T) { t.Parallel() diff --git a/internal/job/config.go b/internal/job/config.go index d96c7f2165..441a2a4fc8 100644 --- a/internal/job/config.go +++ b/internal/job/config.go @@ -64,6 +64,10 @@ type ExecutorConfig struct { // If the commit was part of a pull request, this will container the PR number PullRequest string + // The expected head commit for a pull request build. Intentionally has no + // env tag so hooks cannot change which merge commit the agent accepts. + PullRequestHeadCommit string + // Whether the agent should attempt to checkout the pull request commit using the merge refspec PullRequestUsingMergeRefspec bool diff --git a/internal/job/config_test.go b/internal/job/config_test.go index 64b71b1c5e..b9c12b2072 100644 --- a/internal/job/config_test.go +++ b/internal/job/config_test.go @@ -24,6 +24,7 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) { GitCleanFlags: "-v", GitSSHKey: "original-key", GitRemoteMirrorURL: "https://mirror.example/original.git", + PullRequestHeadCommit: "original-pull-request-head", AgentName: "myAgent", CleanCheckout: false, PluginsAlwaysCloneFresh: false, @@ -40,6 +41,7 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) { "BUILDKITE_CLEAN_CHECKOUT=true", "BUILDKITE_GIT_SSH_KEY=new-key", "BUILDKITE_GIT_REMOTE_MIRROR_URL=https://mirror.example/replaced.git", + "BUILDKITE_PULL_REQUEST_HEAD_COMMIT=replaced-pull-request-head", "BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH=true", "BUILDKITE_GIT_SUBMODULES=true", }) @@ -75,6 +77,9 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) { if got, want := config.GitRemoteMirrorURL, "https://mirror.example/original.git"; got != want { t.Errorf("config.GitRemoteMirrorURL = %q, want immutable %q", got, want) } + if got, want := config.PullRequestHeadCommit, "original-pull-request-head"; got != want { + t.Errorf("config.PullRequestHeadCommit = %q, want immutable %q", got, want) + } if got, want := config.CleanCheckout, true; got != want { t.Errorf("config.CleanCheckout = %t, want %t", got, want) diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index ddbac7268c..00272b3926 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -1399,23 +1399,29 @@ func TestCheckingOutGitHubPullRequestMergeRefspec(t *testing.T) { if err != nil { t.Fatalf("tester.Repo.RevParse(%q) error = %v, want nil", "refs/pull/123/merge", err) } + pullRequestHead, err := tester.Repo.RevParse("refs/pull/123/head") + if err != nil { + t.Fatalf("tester.Repo.RevParse(%q) error = %v, want nil", "refs/pull/123/head", err) + } + pullRequestHead = strings.TrimSpace(pullRequestHead) env := []string{ - "BUILDKITE_GIT_CLONE_FLAGS=--no-local", // Disable the fast local clone method, which automatically copies all refs + "BUILDKITE_GIT_CLONE_FLAGS=--no-local --depth=1", // Disable the fast local clone method, which automatically copies all refs + "BUILDKITE_GIT_FETCH_FLAGS=-v --prune --depth=1", "BUILDKITE_BRANCH=update-test-txt", "BUILDKITE_PULL_REQUEST=123", "BUILDKITE_PIPELINE_PROVIDER=github", + "BUILDKITE_PULL_REQUEST_HEAD_COMMIT=" + pullRequestHead, "BUILDKITE_PULL_REQUEST_USING_MERGE_REFSPEC=true", } - git := tester. - MustMock(t, "git"). - PassthroughToLocalCommand() + git := tester.MustMock(t, "git").PassthroughToLocalCommand() git.ExpectAll([][]any{ - {"clone", "--no-local", "--", tester.Repo.Path, "."}, + {"clone", "--no-local", "--depth=1", "--", tester.Repo.Path, "."}, {"clean", "-ffxdq"}, - {"fetch", "-v", "--prune", "--", "origin", "refs/pull/123/merge"}, + {"fetch", "-v", "--prune", "--depth=1", "--", "origin", "refs/pull/123/merge"}, + {"cat-file", "commit", "FETCH_HEAD"}, {"-c", "advice.detachedHead=false", "checkout", "-f", "FETCH_HEAD"}, {"clean", "-ffxdq"}, {"rev-parse", "FETCH_HEAD"}, @@ -1434,6 +1440,173 @@ func TestCheckingOutGitHubPullRequestMergeRefspec(t *testing.T) { } } +func TestCheckingOutGitHubPullRequestMergeRefspecRetriesStaleHead(t *testing.T) { + t.Parallel() + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + + if err := tester.Repo.CheckoutBranch("update-test-txt"); err != nil { + t.Fatalf("tester.Repo.CheckoutBranch(%q) error = %v", "update-test-txt", err) + } + if _, err := tester.Repo.Execute("reset", "--hard", "main"); err != nil { + t.Fatalf("tester.Repo.Execute(reset --hard main) error = %v", err) + } + if err := os.WriteFile(filepath.Join(tester.Repo.Path, "test.txt"), []byte("This is the force-pushed pull request"), 0o600); err != nil { + t.Fatalf("os.WriteFile(test.txt) error = %v", err) + } + if err := tester.Repo.Add("test.txt"); err != nil { + t.Fatalf("tester.Repo.Add(%q) error = %v", "test.txt", err) + } + if err := tester.Repo.Commit("Force-pushed PR commit"); err != nil { + t.Fatalf("tester.Repo.Commit() error = %v", err) + } + expectedHead, err := tester.Repo.RevParse("HEAD") + if err != nil { + t.Fatalf("tester.Repo.RevParse(%q) error = %v", "HEAD", err) + } + expectedHead = strings.TrimSpace(expectedHead) + if _, err := tester.Repo.Execute("update-ref", "refs/pull/123/head", expectedHead); err != nil { + t.Fatalf("tester.Repo.Execute(update-ref) error = %v", err) + } + if err := tester.Repo.CheckoutBranch("main"); err != nil { + t.Fatalf("tester.Repo.CheckoutBranch(%q) error = %v", "main", err) + } + if _, err := tester.Repo.Execute("merge", "--no-ff", "-m", "Current pull request merge", "update-test-txt"); err != nil { + t.Fatalf("tester.Repo.Execute(merge) error = %v", err) + } + currentMerge, err := tester.Repo.RevParse("HEAD") + if err != nil { + t.Fatalf("tester.Repo.RevParse(%q) error = %v", "HEAD", err) + } + currentMerge = strings.TrimSpace(currentMerge) + if _, err := tester.Repo.Execute("reset", "--hard", "HEAD~1"); err != nil { + t.Fatalf("tester.Repo.Execute(reset --hard HEAD~1) error = %v", err) + } + + var fetches, clones atomic.Int32 + var updatedRemote atomic.Bool + git := tester.MustMock(t, "git").PassthroughToLocalCommand().Before(func(i bintest.Invocation) error { + switch i.Args[0] { + case "clone": + clones.Add(1) + case "fetch": + if slices.Contains(i.Args, "refs/pull/123/merge") { + fetches.Add(1) + } + case "cat-file": + if slices.Equal(i.Args[1:], []string{"commit", "FETCH_HEAD"}) && updatedRemote.CompareAndSwap(false, true) { + if _, err := tester.Repo.Execute("update-ref", "refs/pull/123/merge", currentMerge); err != nil { + return fmt.Errorf("updating simulated GitHub merge ref: %w", err) + } + } + } + return nil + }) + git.Expect().AtLeastOnce().WithAnyArguments() + + env := []string{ + "BUILDKITE_GIT_CLONE_FLAGS=--no-local", + "BUILDKITE_BRANCH=update-test-txt", + "BUILDKITE_PULL_REQUEST=123", + "BUILDKITE_PIPELINE_PROVIDER=github", + "BUILDKITE_PULL_REQUEST_USING_MERGE_REFSPEC=true", + "BUILDKITE_PULL_REQUEST_HEAD_COMMIT=" + expectedHead, + "BUILDKITE_CHECKOUT_ATTEMPTS=2", + } + + if err := tester.Run(t, env...); err != nil { + t.Fatalf("tester.Run() error = %v, want nil after retry. Output:\n%s", err, tester.Output) + } + tester.CheckMocks(t) + + checkoutRepo := &gitRepository{Path: tester.CheckoutDir()} + checkoutCommit, err := checkoutRepo.RevParse("HEAD") + if err != nil { + t.Fatalf("checkoutRepo.RevParse(%q) error = %v", "HEAD", err) + } + if got, want := strings.TrimSpace(checkoutCommit), currentMerge; got != want { + t.Errorf("checked out commit = %q, want current merge %q", got, want) + } + if got, want := fetches.Load(), int32(2); got != want { + t.Errorf("merge-ref fetches = %d, want %d", got, want) + } + if got, want := clones.Load(), int32(1); got != want { + t.Errorf("clones = %d, want %d (stale merge retries must reuse the checkout)", got, want) + } +} + +func TestCheckingOutGitHubPullRequestMergeRefspecExhaustsRetriesForStaleHead(t *testing.T) { + t.Parallel() + + tester, err := NewExecutorTester(mainCtx) + if err != nil { + t.Fatalf("NewExecutorTester() error = %v", err) + } + defer tester.Close() + + expectedHead, err := tester.Repo.RevParse("main") + if err != nil { + t.Fatalf("tester.Repo.RevParse(%q) error = %v", "main", err) + } + expectedHead = strings.TrimSpace(expectedHead) + actualHead, err := tester.Repo.RevParse("refs/pull/123/head") + if err != nil { + t.Fatalf("tester.Repo.RevParse(%q) error = %v", "refs/pull/123/head", err) + } + actualHead = strings.TrimSpace(actualHead) + + var fetches, clones atomic.Int32 + git := tester.MustMock(t, "git").PassthroughToLocalCommand().Before(func(i bintest.Invocation) error { + switch i.Args[0] { + case "clone": + clones.Add(1) + case "fetch": + if slices.Contains(i.Args, "refs/pull/123/merge") { + fetches.Add(1) + } + } + return nil + }) + git.Expect().AtLeastOnce().WithAnyArguments() + + agent := tester.MockAgent(t) + agent.Expect("meta-data", "exists", job.CommitMetadataKey).NotCalled() + + err = tester.Run(t, + "BUILDKITE_GIT_CLONE_FLAGS=--no-local", + "BUILDKITE_BRANCH=update-test-txt", + "BUILDKITE_PULL_REQUEST=123", + "BUILDKITE_PIPELINE_PROVIDER=github", + "BUILDKITE_PULL_REQUEST_USING_MERGE_REFSPEC=true", + "BUILDKITE_PULL_REQUEST_HEAD_COMMIT="+expectedHead, + "BUILDKITE_CHECKOUT_ATTEMPTS=2", + ) + if err == nil { + t.Fatalf("tester.Run() error = nil, want stale merge head failure. Output:\n%s", tester.Output) + } + tester.CheckMocks(t) + + if got, want := fetches.Load(), int32(2); got != want { + t.Errorf("merge-ref fetches = %d, want %d", got, want) + } + if got, want := clones.Load(), int32(1); got != want { + t.Errorf("clones = %d, want %d (stale merge retries must reuse the checkout)", got, want) + } + for _, want := range []string{ + "does not match the build's pull request head", + "expected \"" + expectedHead + "\"", + "got \"" + actualHead + "\"", + } { + if !strings.Contains(tester.Output, want) { + t.Errorf("output does not contain %q. Output:\n%s", want, tester.Output) + } + } +} + func TestCheckingOutGitHubPullRequestAtHeadFromFork(t *testing.T) { t.Parallel()