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

src/cmd/doc_test.go

@@ -73,9 +73,10 @@
 //   - TestServicePushCommand_ProcessFails
 //     Process polling returns FAILED → spinner reports the failure and the
 //     command exits non-zero.
-//   - TestServicePushCommand_SetupNotFoundInYaml_ForwardedToApi
-//     Documents current behavior: a --setup value not present in the local
-//     yaml is forwarded to the API unvalidated.
+//   - TestServicePushCommand_SetupNotFoundInYamlRejectedLocally
+//     A --setup value missing from zerops.yaml is rejected client-side with
+//     an error that names the value and lists the available setups; no API
+//     call is made.
 //
 // ### Tier 2: polling, scope, archive-file-path
 //
@@ -126,25 +127,19 @@
 //   - TestServicePushCommand_GitArchive_DeployGitFolderIncludesGitDir
 //     --deploy-git-folder embeds the .git/ directory in the upload archive.
 //
-// ## Confirmed-bug reproductions (pushDeploy_bugs_test.go)
-//
-// Both tests are kept t.Skip'd so CI stays green; removing the Skip should
-// produce the documented failure (or success, once the underlying bug is
-// fixed) and lock the fix in.
-//
-//   - TestServicePushCommand_SetupFlagOverridesAutoMatch (skipped)
-//     servicePush.go and serviceDeploy.go check the auto-match against the
-//     service name BEFORE consulting the --setup flag. When the service name
-//     happens to equal a setup name in zerops.yaml, the explicit --setup is
-//     silently ignored. Reproduced from a real pipeline running
-//     `--setup showcase-backend` on a service named "backend" with both
-//     setups present. Fix: invert the branches so the flag wins.
-//   - TestServicePushCommand_RunningProcessWithNullAppVersion_BUGPROBE (skipped)
-//     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 the binary crashes with SIGSEGV. Confirmed by running the probe in
-//     isolation. Fix: nil-guard before the deref sites.
+// ## Regression tests for fixed bugs (pushDeploy_bugs_test.go)
+//
+//   - TestServicePushCommand_SetupFlagOverridesAutoMatch
+//     Pins the fix for: servicePush.go / serviceDeploy.go used to check the
+//     auto-match against the service name BEFORE consulting the --setup
+//     flag. When the service name happened to equal a setup name in
+//     zerops.yaml, the explicit --setup was silently ignored. The fix
+//     inverts the branches so the flag wins.
+//   - TestServicePushCommand_RunningProcessWithNullAppVersionNoCrash
+//     Pins the fix for: servicePush.go's log-streaming callback used to
+//     dereference apiProcess.AppVersion.Id and .Build the first time a
+//     poll returned status=RUNNING. AppVersion is a pointer in
+//     output.Process; if the API returned RUNNING before AppVersion was
+//     populated, the CLI nil-derefed in the spinner goroutine and crashed
+//     with SIGSEGV. The fix nil-guards both deref sites.
 package cmd

src/cmd/pushDeploy_bugs_test.go

@@ -2,35 +2,28 @@
 
 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"
 )
 
-// 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.
+// Regression: when the service name matches a setup name in zerops.yaml AND
+// the user passes an explicit --setup, the flag must win. Locks the fix in
+// servicePush.go / serviceDeploy.go that put 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")
 
@@ -50,32 +43,29 @@ func TestServicePushCommand_SetupFlagOverridesAutoMatch(t *testing.T) {
 	assertPushSuccess(t, res, s, "showcase-backend")
 }
 
-// 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).
+// Regression: 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.
 //
-// Builds its own handler set rather than calling registerPushStubs because the
-// /process/{id} response must be call-count dependent and http.ServeMux
+// 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()
@@ -168,12 +158,16 @@ 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

@@ -190,10 +190,11 @@ func TestServicePushCommand_ProcessFails(t *testing.T) {
 	requireNonZeroExit(t, res)
 }
 
-// Current behavior documented: a --setup value not present in zerops.yaml is
-// forwarded verbatim to the API — there is no client-side check that the
-// chosen setup exists in the local yaml.
-func TestServicePushCommand_SetupNotFoundInYaml_ForwardedToApi(t *testing.T) {
+// 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()
@@ -209,7 +210,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 ----------------------------