From f161a73c75910d01f7baab55dafe86f4f48f957b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Hellmann?= Date: Wed, 13 May 2026 14:45:41 +0200 Subject: [PATCH 1/3] fix: honor --setup flag over auto-match by service name --- src/cmd/pushDeploy_bugs_test.go | 23 +++++++---------------- src/cmd/serviceDeploy.go | 17 ++++++++++++----- src/cmd/servicePush.go | 17 ++++++++++++----- 3 files changed, 31 insertions(+), 26 deletions(-) diff --git a/src/cmd/pushDeploy_bugs_test.go b/src/cmd/pushDeploy_bugs_test.go index 3d89fd2..b4b4c56 100644 --- a/src/cmd/pushDeploy_bugs_test.go +++ b/src/cmd/pushDeploy_bugs_test.go @@ -15,23 +15,14 @@ import ( "testing" ) -// 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") diff --git a/src/cmd/serviceDeploy.go b/src/cmd/serviceDeploy.go index fc55f66..0fcc26f 100644 --- a/src/cmd/serviceDeploy.go +++ b/src/cmd/serviceDeploy.go @@ -69,13 +69,20 @@ 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"): + 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 diff --git a/src/cmd/servicePush.go b/src/cmd/servicePush.go index 7ccf844..3fc36c5 100644 --- a/src/cmd/servicePush.go +++ b/src/cmd/servicePush.go @@ -82,13 +82,20 @@ 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"): + 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 From 2643c3ea7f314cdb44d6864c82adbce53b8ac219 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Hellmann?= Date: Wed, 13 May 2026 14:49:59 +0200 Subject: [PATCH 2/3] fix: nil-guard apiProcess.AppVersion in push log-streaming callback --- src/cmd/pushDeploy_bugs_test.go | 51 +++++++++++++++++++-------------- src/cmd/servicePush.go | 9 +++++- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/src/cmd/pushDeploy_bugs_test.go b/src/cmd/pushDeploy_bugs_test.go index b4b4c56..4dd27e8 100644 --- a/src/cmd/pushDeploy_bugs_test.go +++ b/src/cmd/pushDeploy_bugs_test.go @@ -2,10 +2,10 @@ 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" @@ -13,6 +13,8 @@ import ( "net/http" "sync/atomic" "testing" + + "github.com/stretchr/testify/require" ) // TestServicePushCommand_SetupFlagOverridesAutoMatch is a regression lock-in: @@ -43,16 +45,15 @@ 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] @@ -60,16 +61,14 @@ func TestServicePushCommand_SetupFlagOverridesAutoMatch(t *testing.T) { // 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() @@ -162,7 +161,10 @@ 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", @@ -170,5 +172,10 @@ func TestServicePushCommand_RunningProcessWithNullAppVersion_BUGPROBE(t *testing "--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, + ) } diff --git a/src/cmd/servicePush.go b/src/cmd/servicePush.go index 3fc36c5..cc44e45 100644 --- a/src/cmd/servicePush.go +++ b/src/cmd/servicePush.go @@ -229,6 +229,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 ") @@ -253,7 +260,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() { From 48efb37f0282d9577ff59e1d0fce6f119819f51f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Hellmann?= Date: Wed, 13 May 2026 15:00:15 +0200 Subject: [PATCH 3/3] reject unknown --setup client-side before any API call --- src/cmd/serviceDeploy.go | 3 +++ src/cmd/servicePush.go | 3 +++ src/cmd/servicePushDeployShared.go | 21 +++++++++++++++++++++ src/cmd/servicePush_integration_test.go | 19 +++++++++++++------ 4 files changed, 40 insertions(+), 6 deletions(-) diff --git a/src/cmd/serviceDeploy.go b/src/cmd/serviceDeploy.go index 0fcc26f..880754a 100644 --- a/src/cmd/serviceDeploy.go +++ b/src/cmd/serviceDeploy.go @@ -77,6 +77,9 @@ func serviceDeployCmd() *cmdBuilder.Cmd { switch { case cmdData.Params.IsSet("setup"): setup = cmdData.Params.GetString("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 diff --git a/src/cmd/servicePush.go b/src/cmd/servicePush.go index cc44e45..0ecfb51 100644 --- a/src/cmd/servicePush.go +++ b/src/cmd/servicePush.go @@ -90,6 +90,9 @@ func servicePushCmd() *cmdBuilder.Cmd { switch { case cmdData.Params.IsSet("setup"): setup = cmdData.Params.GetString("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 diff --git a/src/cmd/servicePushDeployShared.go b/src/cmd/servicePushDeployShared.go index c5f7b6a..55ffae6 100644 --- a/src/cmd/servicePushDeployShared.go +++ b/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, diff --git a/src/cmd/servicePush_integration_test.go b/src/cmd/servicePush_integration_test.go index 151ce3f..601b807 100644 --- a/src/cmd/servicePush_integration_test.go +++ b/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 ----------------------------