Skip to content

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

Open
l-hellmann wants to merge 3 commits into
cli-testsfrom
fix-push-bugs
Open

Fix --setup handling and nil-deref crash in service push#253
l-hellmann wants to merge 3 commits into
cli-testsfrom
fix-push-bugs

Conversation

@l-hellmann
Copy link
Copy Markdown
Collaborator

@l-hellmann l-hellmann commented May 13, 2026

What Type of Change is this?

  • New Feature
  • Fix
  • Improvement
  • Release
  • Other

Description (required)

Follow-up to #252. Fixes the two confirmed bugs documented there, plus a
related UX improvement uncovered by the first fix. Converts the two
previously-skipped reproductions into live regression tests.

1. --setup flag is ignored when service name matches a yaml setup

servicePush.go and serviceDeploy.go previously checked gn.FindFirst on
the service name before consulting the --setup flag. If a service named
backend had a backend setup in its yaml, an explicit --setup showcase-backend was silently overridden by the auto-match.

Reported from a real pipeline running zcli push backend --setup showcase-backend.

Fix (52f69fa): invert the branches so an explicit flag always wins.
Precedence is now --setup flag > auto-match by service name > interactive
selector (TTY) or hard error (non-TTY).

2. SIGSEGV nil-deref in push log-streaming callback

servicePush.go's log-streaming callback dereferenced
apiProcess.AppVersion.Id and apiProcess.AppVersion.Build on the first
poll that reported status=RUNNING. Both AppVersion and Build are
nullable pointers in output.Process — when the API returned RUNNING
before AppVersion was populated, the CLI nil-deref'd in the spinner
goroutine and crashed with SIGSEGV (Go's runtime can't recover panics from
goroutines you don't own).

Original stack trace, reproduced before the fix:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation]
servicePushCmd.func1.2 (servicePush.go:235)
CheckZeropsProcess.func1 (spinner.go:149)
created by ProcessCheckWithSpinner (spinner.go:48)

Fix (47918ac): nil-guard apiProcess.AppVersion before the deref sites,
plus defer the Build deref behind a Build != nil check.

3. Client-side validation of --setup against the local yaml

Before the precedence fix, the auto-match could mask typos: a --setup typo
on a service whose name happened to match a yaml setup would silently fall
back to the auto-matched setup. After the precedence fix, an unknown
--setup value reaches the server-side yaml validation, which does reject
it: the API responds with errorCode: zeropsYamlSetupNotFound, and the
CLI's validateZeropsYamlContent already maps it through errorsx.Convert
to a user-facing message via i18n.ErrorServiceNotFound -> "Service [name] not found".

That works, but the existing message has two warts:

  • it requires a network round-trip to surface;
  • the i18n string says "Service [...] not found" even though the bad
    value is a setup, which is confusing.

Fix (74c24b1): new helper validateSetupInYaml in
servicePushDeployShared.go rejects unknown values before any API call
with a setup-specific message that also lists what's available:

setup "showcase-backend" was not found in zerops.yaml; available setups: web, db

Test impact

Two previously skipped tests are now live regression guards:

  • TestServicePushCommand_SetupFlagOverridesAutoMatch -- pins the precedence fix
  • TestServicePushCommand_RunningProcessWithNullAppVersionNoCrash
    (renamed from _BUGPROBE) -- pins the nil-deref fix

One existing test was rewritten to match the new contract:

  • TestServicePushCommand_SetupNotFoundInYaml_ForwardedToApi ->
    TestServicePushCommand_SetupNotFoundInYamlRejectedLocally. Asserts
    non-zero exit, error message names the bad value, lists available
    setups, and confirms no upload/deploy API calls were made.

Test plan

Related issues & labels (optional)

Stacked on #252; base will retarget to main automatically after #252 merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant