Fix --setup handling and nil-deref crash in service push

src/cmd/pushDeploy_bugs_test.go

@@ -2,36 +2,29 @@
 
 package cmd
 
-// This file collects integration tests that document confirmed bugs in the
-// push/deploy commands. Each test reproduces a bug with `t.Skip` so CI stays
-// green; remove the t.Skip after the underlying fix lands to lock the
-// regression in.
+// This file collects integration regression tests that pin previously
+// confirmed bugs in the push/deploy commands. Each test reproduces the
+// originally failing scenario and now must pass — they are the live guard
+// against regressions of the corresponding fixes.
 
 import (
 	"encoding/json"
 	"io"
 	"net/http"
 	"sync/atomic"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
-// TestServicePushCommand_SetupFlagOverridesAutoMatch reproduces a confirmed
-// bug: when the service name matches a setup name in zerops.yaml AND the user
-// passes an explicit --setup, the auto-match silently wins and --setup is
-// ignored. Reproduced from a real pipeline running `--setup showcase-backend`
-// on a service named "backend" with both setups present.
-//
-// servicePush.go (and serviceDeploy.go) currently does:
-//
-//	setup, hasMatch := gn.FindFirst(setups, gn.ExactMatch(service.Name.String()))
-//	if !hasMatch { /* only then consult --setup */ }
-//
-// Expected precedence: explicit --setup flag > auto-match by service name >
-// interactive selector (TTY) / hard error (non-TTY). Fix is to invert the
-// branches so the flag is checked first.
+// TestServicePushCommand_SetupFlagOverridesAutoMatch is a regression lock-in:
+// when the service name matches a setup name in zerops.yaml AND the user
+// passes an explicit --setup, the flag must win. Pins the fix in
+// servicePush.go / serviceDeploy.go that puts the flag check before the
+// auto-match by service name. Reproduced from a real pipeline running
+// `--setup showcase-backend` on a service named "backend" with both setups
+// present.
 func TestServicePushCommand_SetupFlagOverridesAutoMatch(t *testing.T) {
-	t.Skip("known bug: --setup is ignored when service name matches a setup in zerops.yaml")
-
 	f := newFixture(t)
 	f.SeedLogin("test-token")
 
@@ -52,33 +45,30 @@ func TestServicePushCommand_SetupFlagOverridesAutoMatch(t *testing.T) {
 	assertPushSuccess(t, res, s, "showcase-backend")
 }
 
-// TestServicePushCommand_RunningProcessWithNullAppVersion_BUGPROBE reproduces
-// a confirmed bug: servicePush.go's log-streaming callback dereferences
-// apiProcess.AppVersion.Id (push.go:235) and apiProcess.AppVersion.Build
-// (push.go:251) the first time a poll returns status=RUNNING. AppVersion is a
-// pointer in output.Process — if the API returns RUNNING before AppVersion is
-// populated, the CLI nil-derefs in the spinner goroutine and crashes the
-// binary (Go's runtime can't recover panics from goroutines you don't own,
-// and ProcessCheckWithSpinner spawns its own goroutine for the poller).
+// TestServicePushCommand_RunningProcessWithNullAppVersionNoCrash is a
+// regression lock-in: servicePush.go's log-streaming callback used to
+// dereference apiProcess.AppVersion.Id and apiProcess.AppVersion.Build on the
+// first poll that reported status=RUNNING. AppVersion is a pointer in
+// output.Process — when the API returned RUNNING before AppVersion was
+// populated, the CLI nil-deref'd in the spinner goroutine and crashed the
+// binary (Go's runtime can't recover panics from goroutines you don't own).
 //
-// Confirmed by removing t.Skip on this test:
+// Original stack trace before the fix:
 //
 //	panic: runtime error: invalid memory address or nil pointer dereference
 //	[signal SIGSEGV: segmentation violation]
 //	github.com/zeropsio/zcli/src/cmd.servicePushCmd.func1.2 (servicePush.go:235)
 //	github.com/zeropsio/zcli/src/uxHelpers.CheckZeropsProcess.func1 (spinner.go:149)
 //	created by ProcessCheckWithSpinner (spinner.go:48)
 //
-// Fix: guard with `if apiProcess.AppVersion == nil { return nil }` before
-// push.go:235; same for AppVersion.Build before :251. Once fixed, drop the
-// t.Skip and this test locks the fix in.
+// The fix nil-guards apiProcess.AppVersion (and separately .Build) before
+// the deref sites; this test drives a RUNNING + appVersion=null poll
+// followed by a FINISHED poll, and the push must complete cleanly.
 //
-// It builds its own handler set rather than calling registerPushStubs because
+// Builds its own handler set rather than calling registerPushStubs because
 // the /process/{id} response must be call-count dependent and http.ServeMux
 // doesn't allow re-registering a pattern.
-func TestServicePushCommand_RunningProcessWithNullAppVersion_BUGPROBE(t *testing.T) {
-	t.Skip("CONFIRMED BUG: push.go:235 nil-deref crashes the binary; remove t.Skip after fix")
-
+func TestServicePushCommand_RunningProcessWithNullAppVersionNoCrash(t *testing.T) {
 	f := newFixture(t)
 	f.SeedLogin("test-token")
 	workDir := t.TempDir()
@@ -171,13 +161,21 @@ func TestServicePushCommand_RunningProcessWithNullAppVersion_BUGPROBE(t *testing
 	})
 
 	// Run WITHOUT --disable-logs so the log-streaming callback fires and the
-	// nil-deref on apiProcess.AppVersion is reached.
+	// nil-deref path is exercised. Before the fix this crashed the test
+	// binary with SIGSEGV; after the fix the callback should bail early on
+	// the null AppVersion and the second poll's FINISHED status completes
+	// the push.
 	res := f.Run(
 		nil,
 		"service", "push",
 		"--service-id", pushServiceID,
 		"--working-dir", workDir,
 		"--no-git",
 	)
-	t.Logf("exit=%d stderr=%q", res.ExitCode, res.Stderr)
+	require.Equalf(
+		t,
+		0,
+		res.ExitCode,
+		"push should complete cleanly even with appVersion=null on first poll\nstderr=%q", res.Stderr,
+	)
 }

src/cmd/serviceDeploy.go

@@ -69,13 +69,23 @@ func serviceDeployCmd() *cmdBuilder.Cmd {
 				return err
 			}
 
-			setup, hasMatch := gn.FindFirst(setups, gn.ExactMatch(service.Name.String()))
-			if !hasMatch {
+			// Precedence: explicit --setup flag > auto-match by service name >
+			// interactive selector (TTY) / hard error (non-TTY). The flag must
+			// be consulted first; otherwise a service whose name happens to
+			// equal one of the yaml setups silently ignores the user's --setup.
+			var setup string
+			switch {
+			case cmdData.Params.IsSet("setup"):
 				setup = cmdData.Params.GetString("setup")
-				switch {
-				case !terminal.IsTerminal() && !cmdData.Params.IsSet("setup"):
+				if err := validateSetupInYaml(setup, setups); err != nil {
+					return err
+				}
+			default:
+				if match, hasMatch := gn.FindFirst(setups, gn.ExactMatch(service.Name.String())); hasMatch {
+					setup = match
+				} else if !terminal.IsTerminal() {
 					return errors.New("Cannot find corresponding setup in zerops.yaml, please select with --setup")
-				case !cmdData.Params.IsSet("setup"):
+				} else {
 					setup, err = uxHelpers.PrintSetupSelector(ctx, setups)
 					if err != nil {
 						return err

src/cmd/servicePush.go

@@ -82,13 +82,23 @@ func servicePushCmd() *cmdBuilder.Cmd {
 				return err
 			}
 
-			setup, hasMatch := gn.FindFirst(setups, gn.ExactMatch(service.Name.String()))
-			if !hasMatch {
+			// Precedence: explicit --setup flag > auto-match by service name >
+			// interactive selector (TTY) / hard error (non-TTY). The flag must
+			// be consulted first; otherwise a service whose name happens to
+			// equal one of the yaml setups silently ignores the user's --setup.
+			var setup string
+			switch {
+			case cmdData.Params.IsSet("setup"):
 				setup = cmdData.Params.GetString("setup")
-				switch {
-				case !terminal.IsTerminal() && !cmdData.Params.IsSet("setup"):
+				if err := validateSetupInYaml(setup, setups); err != nil {
+					return err
+				}
+			default:
+				if match, hasMatch := gn.FindFirst(setups, gn.ExactMatch(service.Name.String())); hasMatch {
+					setup = match
+				} else if !terminal.IsTerminal() {
 					return errors.New("Cannot find corresponding setup in zerops.yaml, please select with --setup")
-				case !cmdData.Params.IsSet("setup"):
+				} else {
 					setup, err = uxHelpers.PrintSetupSelector(ctx, setups)
 					if err != nil {
 						return err
@@ -222,6 +232,13 @@ func servicePushCmd() *cmdBuilder.Cmd {
 									if !apiProcess.Status.IsRunning() {
 										return nil
 									}
+									// AppVersion is a nullable pointer; the API can briefly
+									// report status=RUNNING before it's populated. Without
+									// this guard, the derefs below crash the binary from
+									// inside the spinner goroutine.
+									if apiProcess.AppVersion == nil {
+										return nil
+									}
 									if logsHandler == nil {
 										pipelineLink := styles.NewStringBuilder()
 										pipelineLink.WriteInfoColor("View full pipeline at ")
@@ -246,7 +263,7 @@ func servicePushCmd() *cmdBuilder.Cmd {
 											cmdData.RestApiClient,
 										)
 									}
-									if !buildPhase {
+									if !buildPhase && apiProcess.AppVersion.Build != nil {
 										buildPhase = true
 										buildServiceId, _ := apiProcess.AppVersion.Build.ServiceStackId.Get()
 										go func() {

src/cmd/servicePushDeployShared.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/pkg/errors"
 	"github.com/zeropsio/zcli/src/entity"
@@ -102,6 +103,26 @@ func packageStream(ctx context.Context, uploadUrl types.String, reader io.Reader
 	return nil
 }
 
+// validateSetupInYaml ensures that an explicitly requested setup name exists
+// in the local zerops.yaml. The API rejects unknown setups too, but a
+// client-side check produces a faster, friendlier error that lists the
+// available setups instead of a generic ZeropsYamlSetupNotFound from the
+// server.
+func validateSetupInYaml(setup string, setups []string) error {
+	if setup == "" {
+		return nil
+	}
+	for _, s := range setups {
+		if s == setup {
+			return nil
+		}
+	}
+	if len(setups) == 0 {
+		return errors.Errorf("setup %q was not found in zerops.yaml: the file has no setups defined", setup)
+	}
+	return errors.Errorf("setup %q was not found in zerops.yaml; available setups: %s", setup, strings.Join(setups, ", "))
+}
+
 func validateZeropsYamlContent(
 	ctx context.Context,
 	restApiClient *zeropsRestApiClient.Handler,

src/cmd/servicePush_integration_test.go

@@ -207,11 +207,12 @@ func TestServicePushCommand_ProcessFails(t *testing.T) {
 	requireNonZeroExit(t, res)
 }
 
-// TestServicePushCommand_SetupNotFoundInYaml_ForwardedToApi documents current
-// behavior: a --setup value not present in zerops.yaml is forwarded verbatim
-// to the API, with no client-side check that the chosen setup exists in the
-// local yaml.
-func TestServicePushCommand_SetupNotFoundInYaml_ForwardedToApi(t *testing.T) {
+// TestServicePushCommand_SetupNotFoundInYamlRejectedLocally checks that a
+// --setup value not present in zerops.yaml is rejected client-side before any
+// API call. The error message names the offending value and lists the
+// available setups so the user can correct a typo without round-tripping
+// through the server.
+func TestServicePushCommand_SetupNotFoundInYamlRejectedLocally(t *testing.T) {
 	f := newFixture(t)
 	f.SeedLogin("test-token")
 	workDir := t.TempDir()
@@ -228,7 +229,13 @@ func TestServicePushCommand_SetupNotFoundInYaml_ForwardedToApi(t *testing.T) {
 		"--no-git",
 		"--disable-logs",
 	)
-	assertPushSuccess(t, res, s, "nonexistent")
+	requireNonZeroExit(t, res)
+	assert.Contains(t, res.Stderr, "nonexistent", "stderr should name the missing setup")
+	assert.Contains(t, res.Stderr, "web", "stderr should list available setups")
+	assert.Contains(t, res.Stderr, "db", "stderr should list available setups")
+	// No API request should have followed the local rejection.
+	assert.Zero(t, s.uploadBytes.Load(), "upload should not happen on local validation failure")
+	assert.Equal(t, int32(0), s.deployHits.Load(), "deploy endpoint must not be hit")
 }
 
 // --- Tier 2: polling, scope, archive-file-path ----------------------------