fix: exit non-zero when a command fails - #277

@leechenghsiu leechenghsiu commented Sep 6, 2026 •

edited

Loading

Copy link

Copy Markdown

Contributor

Summary

cmd/main.go logged the error from rootCmd.Execute() and then returned normally, so every failure exited 0: unknown command, unknown flag, API 401, missing workspace. Shells (set -e, &&), CI and the agent sandbox's bash tool all key off the exit status, so failed zeabur calls looked like successes. fix(npm): mirror the binary's exit status instead of throwing a Node stack trace #276 made the npm wrapper mirror the binary's status, which exposed this.
Fix: os.Exit(1) after logging. Output format is unchanged (same ERROR line on stderr, --json unaffected). Commands that already set their own status (service exec, server exec forward the remote exit code) are untouched.

Test plan

make test, make build pass
Built binary, ZEABUR_TOKEN=zat_bogus, clean HOME: bogus-command → 1, version --no-such-flag → 1, profile info (401) → 1, project list --workspace does-not-exist → 1, project list (401) → 1, version → 0
After release: npx zeabur@latest bogus-command; echo $? prints 1

Noticed, not fixed here

With no token and -i=false, zeabur profile info blocks waiting for the browser login callback instead of failing fast. Non-interactive mode should error out; separate issue.

Generated with Claude Code

@leechenghsiu leechenghsiu self-assigned this

@linear-code

linear-code Bot commented Sep 6, 2026

Copy link

Copy Markdown

DES-913

opencodezebra Bot commented Sep 6, 2026

Copy link

Copy Markdown

Review Council started (round 1).

Baseline:

Scope: 2 files changed (+77/−1)
CI/checks: pending (0 contexts) at 5fb8b42

The council is reviewing this pull request; the verdict will follow as a separate comment when the round closes.

opencodezebra Bot commented Sep 6, 2026

Copy link

ばつ0 ×ばつ0 ×ばつ1 · 💬 Comment `@opencodezebra <question>` for a follow-up · 🔁 Push new commits or comment `@opencodezebra review <fix notes>` to re-run the council  " data-view-component="true"> Copy Markdown

LGTM ✅ — A one-line fix that makes zeabur exit non-zero on failure, with a real end-to-end test and no defects found.
Reviewed at 5fb8b42 (round 1)

What This PR Does

cmd/main.go previously logged the error returned by rootCmd.Execute() and then fell through to a normal return, so every failure — unknown command, unknown flag, API 401, missing workspace — exited with status 0. Shells (set -e, &&), CI, and the agent sandbox's bash tool all key off the process exit status, so failed zeabur invocations looked like successes. This PR adds os.Exit(1) after the error is logged, and adds cmd/main_test.go which builds the real binary and asserts its process exit code. Refs DES-913; follows #276 (DES-909) which made the npm wrapper mirror the binary's status and thereby exposed this bug.

How It Works

cmd/main.go: after factory.Log.Error(err) in the rootCmd.Execute() error branch, the process now calls os.Exit(1). Output format is unchanged (same ERROR line on stderr; --json unaffected), and commands that forward their own remote exit code (service exec, server exec) are untouched.
cmd/main_test.go: compiles the actual entrypoint into a temp dir and runs it as a subprocess, asserting unknown command → non-zero with the error on stderr, unknown flag → non-zero, and version → 0. Subprocesses set HOME=t.TempDir() and clear ZEABUR_TOKEN to isolate the developer's login.

Findings

ID	Severity	Finding	Location
F1	🟢	Test isolates config via HOME, but Go's os.UserConfigDir() uses %AppData% on Windows; harmless today since CI is ubuntu-latest-only (verified, no OS matrix), but would touch a real dev's config if run on Windows (raised by: rev-claude)	cmd/main_test.go:32

Finding Details

🟢 F1: Windows config isolation via HOME is a no-op on Windows

rev-claude noted that the test isolates config with HOME=t.TempDir(), but on Windows os.UserConfigDir() reads %AppData% rather than HOME, so the isolation would not hold there. This does not bite in CI: the chair verified .github/workflows/build-test.yml runs a single build-test job on runs-on: ubuntu-latest with no OS matrix. It requires no action in this PR and is recorded only as context for anyone who later adds a Windows runner. Left as a 🟢 note rather than a blocking finding.

What's Good (🟢)

Correct, minimal fix: a single os.Exit(1) on the existing error path resolves the reported exit-code bug (rev-claude, rev-codex).
The test builds and runs the real entrypoint as a subprocess, so it exercises the actual process exit code rather than a mocked return — the right approach for this specific fix (rev-claude).
Output contract preserved: same ERROR stderr line, --json unaffected; commands that forward remote exit codes are untouched (rev-codex).
No security impact: the change adds only a non-zero exit after the existing log; it does not alter authentication, authorization, command routing, or error contents (rev-codex).
zap's stderr sink writes synchronously, so factory.Log.Error(err) completes before os.Exit(1) — no truncated output (rev-claude).

Baseline Check

Main already has: the error-logging branch in cmd/main.go; no prior cmd/ tests to conflict with.
Net-new value: failures now propagate a non-zero exit status; regression test guards it.
CI state: no statuses reported yet at head 5fb8b42 (pending); make test / make build run on ubuntu-latest.
No prior council verdict comment — first-round review.

Review Metadata

Reviewers: rev-codex (security — approve), rev-claude (correctness — approve, one non-blocking Windows nit)
Consensus: approve
Absent reviewers: none

×ばつ0 ×ばつ0 ×ばつ1 · 💬 Comment @opencodezebra <question> for a follow-up · 🔁 Push new commits or comment @opencodezebra review <fix notes> to re-run the council

opencodezebra[bot]

opencodezebra Bot approved these changes

View reviewed changes

@opencodezebra opencodezebra Bot left a comment

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Council approve — 🔴0 🟡0 🟢1. Reviewed at 5fb8b42. Full report: #277 (comment)

@leechenghsiu @claude


 fix: exit non-zero when a command fails

c9e038b

cmd/main.go logged the error returned by rootCmd.Execute() and then fell off
the end of main, so every failure — unknown command, bad flag, 401 from the
API, missing workspace — exited 0. Shells (`set -e`, `&&`), CI and the agent
sandbox's bash tool all read the exit status, so a failed `zeabur` call was
indistinguishable from a successful one.
Exit 1 after logging. Output is unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

leechenghsiu force-pushed the matthewlee/des-913-zeabur-cli-發生錯誤時退出碼仍為-0,agent-與腳本無法判斷失敗 branch from 5fb8b42 to c9e038b Compare

September 6, 2026 15:11

opencodezebra Bot commented Sep 6, 2026

Copy link

Copy Markdown

Review Council started (round 2).

Baseline:

Scope: 1 files changed (+2/−1)
CI/checks: pending (0 contexts) at c9e038b

The council is reviewing this pull request; the verdict will follow as a separate comment when the round closes.

opencodezebra Bot commented Sep 6, 2026

Copy link

ばつ0 ×ばつ0 ×ばつ2 · 💬 Comment `@opencodezebra <question>` for a follow-up · 🔁 Push new commits or comment `@opencodezebra review <fix notes>` to re-run the council  " data-view-component="true"> Copy Markdown

LGTM ✅ — The exit-code fix is correct and CI-green; the only change from round 1 is that the regression test was dropped, which both reviewers flag as non-blocking.
Reviewed at c9e038b (round 2)

Delta since `5fb8b42` (round 1)

The branch was force-pushed to a single rewritten commit c9e038b; the round-1 reviewed SHA 5fb8b42 is not an ancestor of the current head (branch amended/rebased, not fast-forwarded). The broker cannot run merge-base, so per protocol this round is a full review scoped to the current single-commit diff, not an incremental one.
The fix itself is unchanged: os.Exit(1) after factory.Log.Error(err) on the rootCmd.Execute() error path in cmd/main.go (+2/−1).
cmd/main_test.go — the subprocess regression test round 1 added and the council praised — was removed. cmd/ now contains only main.go (chair-verified via the PR-head tree).
Round-1 F1 (🟢 Windows config isolation in the test) is moot: the test it targeted no longer exists.

Findings

Resolved	Severity	Finding	Fixed in
F1	🟢	Test isolated config via HOME, a no-op on Windows — moot now that cmd/main_test.go was removed	`c9e038b`

Outstanding	Severity	Finding	Location
—	—	None	—

New	Severity	Finding	Location
F2	🟢	Round 1's regression test for exit-status was dropped in the rewrite; fix stays correct but is no longer covered by an automated test (raised by: rev-claude)	cmd/main.go (no cmd/main_test.go present)

Finding Details

🟢 F2: Regression test for the exit-code behavior was dropped

Round 1 shipped cmd/main_test.go, a subprocess test asserting unknown-command/flag → non-zero and version → 0, and the round-1 council approved specifically citing that end-to-end test. The current head c9e038b removes that file — the chair confirmed via the PR-head tree that cmd/ now contains only main.go. The one-line fix is still correct (both reviewers verified it, and the exec commands manage their own exit code so no integration conflict exists), but the exact behavior this PR fixes — exit-status correctness for scripts/CI — is no longer guarded by a test. rev-claude recommends re-adding cmd/main_test.go before merge. Recorded as a non-blocking note (rev-claude marked it non-blocking; it names no defect in shipped behavior), not a gate: a correct, CI-green one-line fix should not be blocked on test coverage, but re-adding the test would keep the fixed behavior from silently regressing.

What's Good (🟢)

Correct, minimal fix: a single os.Exit(1) on the existing error path resolves the exit-code masking bug (rev-codex, rev-claude).
No integration conflict: service exec / server exec call os.Exit(<remote code>) themselves and return nil on success, so the new exit never fires on their paths and never masks remote exit codes (rev-claude, chair-verified in internal/cmd/service/exec/exec.go).
Logging completes before exit: os.Exit(1) runs after factory.Log.Error(err) returns, with no defer in main() skipped — no truncated output (rev-claude).
No security impact: adds only a non-zero exit after the existing log; credentials, authorization, command routing, and error contents are unchanged (rev-codex).
Output contract preserved: same ERROR stderr line, --json unaffected.

Baseline Check

Main already has: the error-logging branch in cmd/main.go.
Net-new value: failures now propagate a non-zero exit status so shells, CI, and the agent sandbox can distinguish failure from success.
CI state at c9e038b: build-test ✅, lint ✅; CodeQL (Analyze go) reported green by rev-claude (in-progress at chair's earlier check); codesmith skipped.
Prior round: round-1 LGTM at 5fb8b42 (rebased away); this round is a full re-review of the rewritten single commit.

Review Metadata

Reviewers: rev-codex (security — approve, no findings), rev-claude (correctness/integration — approve, one non-blocking test-coverage note)
Consensus: approve
Absent reviewers: none

×ばつ0 ×ばつ0 ×ばつ2 · 💬 Comment @opencodezebra <question> for a follow-up · 🔁 Push new commits or comment @opencodezebra review <fix notes> to re-run the council

opencodezebra[bot]

opencodezebra Bot approved these changes

View reviewed changes

@opencodezebra opencodezebra Bot left a comment

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Council approve — 🔴0 🟡0 🟢2. Reviewed at c9e038b. Full report: #277 (comment)

leechenghsiu merged commit 4147c0b into main

6 checks passed

leechenghsiu deleted the matthewlee/des-913-zeabur-cli-發生錯誤時退出碼仍為-0,agent-與腳本無法判斷失敗 branch

September 6, 2026 15:14

Labels

None yet

1 participant