Don't run pre-exit hook without command phase

internal/job/executor.go

@@ -13,6 +13,7 @@ import (
 	"path/filepath"
 	"regexp"
 	"runtime"
+	"slices"
 	"strconv"
 	"strings"
 	"time"
@@ -180,30 +181,18 @@ func (e *Executor) Run(ctx context.Context) (exitCode int) {
 		return shell.GetExitCode(err)
 	}
 
-	includePhase := func(phase string) bool {
-		if len(e.Phases) == 0 {
-			return true
-		}
-		for _, include := range e.Phases {
-			if include == phase {
-				return true
-			}
-		}
-		return false
-	}
-
 	// Execute the job phases in order
 	var phaseErr error
 
-	if includePhase("plugin") {
+	if e.includePhase("plugin") {
 		phaseErr = e.preparePlugins()
 
 		if phaseErr == nil {
 			phaseErr = e.PluginPhase(ctx)
 		}
 	}
 
-	if phaseErr == nil && includePhase("checkout") {
+	if phaseErr == nil && e.includePhase("checkout") {
 		phaseErr = e.CheckoutPhase(ctx)
 	} else {
 		checkoutDir, exists := e.shell.Env.Get("BUILDKITE_BUILD_CHECKOUT_PATH")
@@ -212,11 +201,11 @@ func (e *Executor) Run(ctx context.Context) (exitCode int) {
 		}
 	}
 
-	if phaseErr == nil && includePhase("plugin") {
+	if phaseErr == nil && e.includePhase("plugin") {
 		phaseErr = e.VendoredPluginPhase(ctx)
 	}
 
-	if phaseErr == nil && includePhase("command") {
+	if phaseErr == nil && e.includePhase("command") {
 		var commandErr error
 		phaseErr, commandErr = e.CommandPhase(ctx)
 		/*
@@ -274,6 +263,13 @@ func (e *Executor) Run(ctx context.Context) (exitCode int) {
 	return exitStatusCode
 }
 
+func (e *Executor) includePhase(phase string) bool {
+	if len(e.Phases) == 0 {
+		return true
+	}
+	return slices.Contains(e.Phases, phase)
+}
+
 // Cancel interrupts any running shell processes and causes the job to stop
 func (e *Executor) Cancel() error {
 	e.cancelCh <- struct{}{}
@@ -738,16 +734,24 @@ func (e *Executor) tearDown(ctx context.Context) error {
 	var err error
 	defer func() { span.FinishWithError(err) }()
 
-	if err = e.executeGlobalHook(ctx, "pre-exit"); err != nil {
-		return err
-	}
+	// In vanilla agent usage, there's always a command phase.
+	// But over in agent-stack-k8s, which splits the agent phases among
+	// containers (the checkout phase happens in a separate container to the
+	// command phase), the two phases have different environments.
+	// Unfortunately pre-exit hooks are often not written with this split in
+	// mind.
+	if e.includePhase("command") {
+		if err = e.executeGlobalHook(ctx, "pre-exit"); err != nil {
+			return err
+		}
 
-	if err = e.executeLocalHook(ctx, "pre-exit"); err != nil {
-		return err
-	}
+		if err = e.executeLocalHook(ctx, "pre-exit"); err != nil {
+			return err
+		}
 
-	if err = e.executePluginHook(ctx, "pre-exit", e.pluginCheckouts); err != nil {
-		return err
+		if err = e.executePluginHook(ctx, "pre-exit", e.pluginCheckouts); err != nil {
+			return err
+		}
 	}
 
 	// Support deprecated BUILDKITE_DOCKER* env vars

internal/job/integration/hooks_integration_test.go

@@ -322,6 +322,21 @@ func TestPreExitHooksFireAfterCommandFailures(t *testing.T) {
 	tester.CheckMocks(t)
 }
 
+func TestPreExitHooksDoesNotFireWithoutCommandPhase(t *testing.T) {
+	t.Parallel()
+
+	tester, err := NewBootstrapTester(mainCtx)
+	if err != nil {
+		t.Fatalf("NewBootstrapTester() error = %v", err)
+	}
+	defer tester.Close()
+
+	tester.ExpectGlobalHook("pre-exit").NotCalled()
+	tester.ExpectLocalHook("pre-exit").NotCalled()
+
+	tester.RunAndCheck(t, "BUILDKITE_BOOTSTRAP_PHASES=plugin,checkout")
+}
+
 func TestPreExitHooksFireAfterHookFailures(t *testing.T) {
 	t.Parallel()