fix(npm): mirror the binary's exit status instead of throwing a Node stack trace - #276

@leechenghsiu leechenghsiu commented Sep 6, 2026 •

edited

Loading

Copy link

Copy Markdown

Contributor

Summary

npm/index.js (what npx zeabur runs) let execFileSync throw on any non-zero exit: Node printed an uncaught-exception stack trace with stdout: null, stderr: null (stdio is inherited, so the real output had already been printed) and the exit code collapsed to 1. An agent reading that concluded "exec produced nothing" instead of seeing psql's real role "postgres" does not exist (exit 2) — DES-909.
The wrapper now mirrors the binary's exit status; a signal death exits 128 + signal; a spawn failure (missing binary) prints one line and exits 1. stdio passes through exactly as before.

Test plan

Manually staged index.js next to a fake binary: exit 2 → exit code 2 with only the binary's stderr; success → exit 0 with stdout/stderr passed through; missing binary → one-line message, exit 1
make build, make test unchanged (the wrapper is not part of the Go build)
After the next release: npx zeabur@latest service exec ... -- sh -c 'exit 2' returns 2 with no Node stack trace

Refs DES-909

Generated with Claude Code

@leechenghsiu leechenghsiu self-assigned this

@linear-code

linear-code Bot commented Sep 6, 2026

Copy link

Copy Markdown

DES-909

opencodezebra Bot commented Sep 6, 2026 •

edited

Loading

Copy link

Copy Markdown

Review Council round closed without a verdict — superseded by a newer round or timed out.

opencodezebra Bot commented Sep 6, 2026

Copy link

Copy Markdown

Review Council started (round 1).

Baseline:

Scope: 5 files changed (+94/−2)
CI/checks: pending (0 contexts) at 2ee029f

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 ×ばつ4 · 💬 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 tightly-scoped npm-wrapper fix that mirrors the binary's real exit status; both reviewers approve with no actionable findings and CI is green.
Reviewed at 2ee029f (round 1)

What This PR Does

npm/index.js is the shim npx zeabur runs. Previously it let execFileSync throw on any non-zero exit, so Node printed an uncaught-exception stack trace (stdout: null, stderr: null, since stdio is inherited the real output had already been shown) and collapsed the child's exit code to 1 — masking the binary's actual failure (DES-909). The wrapper now catches the error and mirrors the child's exit status: a numeric status is propagated as-is, a signal death exits 128 + signal, and a spawn failure (missing binary) prints one line and exits 1. A new behavioural test (npm/index.test.mjs) and a CI step exercise these paths; two .gitignore edits keep the test tracked in git but excluded from the published tarball.

How It Works

npm/index.js: wraps execFileSync in try/catch — typeof e.status === "number" → process.exit(e.status); e.signal → process.exit(128 + os.constants.signals[e.signal] ?? 0); else console.error(e.message) + exit(1). Success path (stdio: "inherit") unchanged.
npm/index.test.mjs: stages index.js next to a fake /bin/sh binary and asserts stdout/stderr pass-through and exit codes for success, exit 2, and SIGTERM → 143.
.github/workflows/build-test.yml: adds setup-node@v4 (SHA-pinned) + node --test npm/.
.gitignore / npm/.gitignore: un-ignore index.test.mjs for git; publish allowlist (.npmignore) unchanged so npm pack still excludes it.

Findings

ID	Severity	Finding	Location
F1	🟢	Error-object branch mapping verified against Node's execFileSync semantics (numeric status / signal name / ENOENT fall-through) (raised by: rev-claude)	npm/index.js:43
F2	🟢	execFileSync still runs a fixed package-relative binary with no shell — command-injection resistance preserved; new setup-node action is full-SHA pinned (raised by: rev-codex)	.github/workflows/build-test.yml:26
F3	🟢	Test tracked in git via npm/.gitignore but excluded from the tarball via unchanged .npmignore; node --test only discovers *.test.mjs so index.js isn't run as a test (raised by: rev-claude)	npm/.gitignore:6
F4	🟢	Unrelated .worktrees/ entry added to root .gitignore — harmless, minor scope creep outside the wrapper fix (raised by: rev-claude)	.gitignore:18

Finding Details

🟢 F1: Exit-status branch mapping is correct

rev-claude traced Node's execFileSync→checkExecSyncError semantics: a non-zero child exit yields a numeric err.status (first branch); a signal kill yields err.status === null with a string err.signal (second branch, 128 + os.constants.signals[e.signal], matching the SIGTERM → 143 test and shell convention); a spawn error (ENOENT) leaves both status and signal null, correctly falling through to console.error + exit(1). Behaviour matches the PR's stated intent.

🟢 F2: No security regression in the changed surface

rev-codex confirmed arguments stay argv-separated through execFileSync (no shell), executable resolution is a fixed package-relative path not redirectable by user input, the new error path discloses only e.message (no secrets), and the added third-party action is pinned to a full commit SHA. No supply-chain or injection exposure.

🟢 F3: Packaging and CI wiring are correct

The test is un-ignored in npm/.gitignore so it is tracked in git, while the publish allowlist (npm/.npmignore, unchanged) has no entry for it, so npm pack excludes it — consistent with the PR's npm pack --dry-run claim. node --test npm/ only auto-discovers *.test.mjs, so index.js is not mistaken for a test. The staged fake-binary path in the harness mirrors the wrapper's own platform/arch mapping.

🟢 F4: Minor out-of-scope churn

The root .gitignore gains a .worktrees/ entry unrelated to the exit-code fix. Harmless and needs no action in this PR; noted only for awareness. rev-claude also observed make test still runs only go test ./..., so the new JS test runs via CI only — a small local-parity gap, not a defect.

What's Good (🟢)

Fix is minimal and transparent: success path and inherited stdio are untouched.
New test faithfully exercises all three failure branches against real Node error shapes.
CI step is correctly wired and the test is kept out of the published tarball.
Third-party GitHub Action is SHA-pinned; no shell/command-injection surface introduced.

Baseline Check

Main already has: the npm wrapper invoking execFileSync with inherited stdio.
Net-new value: exit-status fidelity (numeric / signal / spawn-failure) + regression test + CI coverage.
CI at reviewed head: CodeQL ✅, Analyze (go) ✅, build-test ✅, lint ✅, [code]smith skipped — all green (both reviewers noted CI in-progress at review time; the chair re-verified all checks passed at the same head).
Prior round: an earlier council round closed without a verdict (superseded/timed out); no findings to carry, so this is a fresh round-1 review.

Review Metadata

Reviewers: rev-codex (security) — approve; rev-claude (correctness/integration) — approve
Consensus: approve
Absent reviewers: none

×ばつ0 ×ばつ0 ×ばつ4 · 💬 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 🟢4. Reviewed at 2ee029f. Full report: #276 (comment)

leechenghsiu force-pushed the matthewlee/des-909-zeabur-npm-wrapper-在-binary-非零退出時噴-node-stack-trace-並蓋掉退出碼 branch from 2ee029f to f8125cc Compare

September 6, 2026 14:42

opencodezebra Bot commented Sep 6, 2026 •

edited

Loading

Copy link

Copy Markdown

Review Council round closed without a verdict — superseded by a newer round or timed out.

@leechenghsiu @claude


 fix(npm): mirror the binary's exit status instead of throwing a Node ...

729d3bd

...stack trace
`npx zeabur ...` runs npm/index.js, which spawned the Go binary with
execFileSync and never caught the throw on a non-zero exit. Two things went
wrong every time the CLI failed: Node printed an uncaught-exception stack trace
whose error object carried `stdout: null, stderr: null` (stdio is inherited, so
the real output had already gone to the terminal), and the process exit code
became 1 regardless of what the binary returned. An agent reading that saw
"exec produced nothing" instead of psql's actual `role "postgres" does not
exist` (exit 2).
The wrapper now exits with the binary's status; a signal death maps to the
shell convention 128+signal; a spawn failure (e.g. missing binary) prints one
line and exits 1.
Refs DES-909
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

leechenghsiu force-pushed the matthewlee/des-909-zeabur-npm-wrapper-在-binary-非零退出時噴-node-stack-trace-並蓋掉退出碼 branch from f8125cc to 729d3bd Compare

September 6, 2026 14:44

opencodezebra Bot commented Sep 6, 2026

Copy link

Copy Markdown

Review Council started (round 2).

Baseline:

Scope: 1 files changed (+17/−1)
CI/checks: pending (0 contexts) at 729d3bd

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

leechenghsiu merged commit 630f10a into main

5 checks passed

leechenghsiu deleted the matthewlee/des-909-zeabur-npm-wrapper-在-binary-非零退出時噴-node-stack-trace-並蓋掉退出碼 branch

September 6, 2026 14:46

opencodezebra Bot commented Sep 6, 2026

Copy link

ばつ0 ×ばつ1 ×ばつ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

CHANGES REQUESTED ⚠️ — The index.js exit-status fix is correct and unchanged, but the branch was rewritten to drop the regression test and CI wiring that round 1 approved, removing automated coverage for exactly the bug this PR fixes.
Reviewed at 729d3bd (round 2)

Delta since `2ee029f` (prior round, unreachable)

The prior reviewed head 2ee029f is not in this PR's commit history — the branch was squashed/force-pushed to a single commit, so 2ee029f is unreachable and this round was reviewed in full rather than incrementally.

Diff shrank from 5 files (+94/−2) to 1 file (+17/−1): only npm/index.js remains.
npm/index.test.mjs (the behavioural regression test) was removed — confirmed absent at head.
The node --test npm/ CI step and setup-node action were removed — build-test.yml at head runs only make test + make build (Go).
Both root .gitignore and npm/.gitignore edits were dropped.
npm/index.js itself is byte-identical to the round-1 approved version.

Findings

ID	Severity	Finding	Location
F5	🟡	Regression test + CI wiring that covered the exit/signal/spawn-failure branches were dropped when the branch was rewritten; no automated coverage remains for the bug this PR fixes (raised by: rev-claude)	.github/workflows/build-test.yml:17
F1	🟢	Exit-status branch mapping (numeric status / signal → 128+signal / spawn-failure fall-through) is correct and unchanged from round 1 (raised by: rev-claude)	npm/index.js:47
F2	🟢	No security regression: fixed package-relative binary, no shell, argv-separated args, error path discloses only e.message (raised by: rev-codex)	npm/index.js:48

Finding Details

🟡 F5: Regression test and CI coverage removed by the branch rewrite

rev-claude found — and the chair independently confirmed — that npm/index.test.mjs (404 at head 729d3bd) and the JS test wiring are gone. .github/workflows/build-test.yml at this head runs only make test and make build (Go); there is no node --test npm/ step and no setup-node action. The round-1 verdict was premised on a diff that included these; the rewrite dropped them, leaving no automated regression protection for the exit-status/signal/spawn-failure behaviour this PR exists to fix. The PR body's test plan is now manual-only, which is consistent with the diff but a genuine coverage loss.

Concrete action: restore npm/index.test.mjs and the node --test npm/ CI step (as in the round-1 head), or state explicitly that dropping them is intentional. This is not a code defect — index.js is correct — but a re-introduced gap relative to the previously-approved scope.

🟢 F1: Exit-status branch mapping is correct (unchanged)

Both reviewers re-confirmed the try/catch partitions Node's execFileSync error shapes correctly: numeric e.status → propagated; e.signal → 128 + (os.constants.signals[e.signal] ?? 0) (shell convention, degrades gracefully for unknown signals); ENOENT/spawn failure → console.error(e.message) + exit(1). process.exit() terminates synchronously so branches do not fall through. Byte-identical to the round-1 approved code.

🟢 F2: No security regression

rev-codex verified the wrapper still invokes a fixed package-relative binary with no shell, user arguments stay argv-separated, executable selection is not user-redirectable, and the exception path discloses only the ordinary error message — no stack-trace or secret exposure, no fail-open.

What's Good (🟢)

The core fix is minimal and transparent: success path and inherited stdio are untouched; only the failure path changed.
Exit-status fidelity (numeric / signal / spawn-failure) correctly mirrors the child process per shell convention.
No security or command-injection surface introduced; index.js is unchanged from the round-1 approved version.

Baseline Check

Main already has: the npm wrapper invoking execFileSync with inherited stdio.
Net-new value: exit-status fidelity for npx zeabur failures (DES-909).
CI at head 729d3bd: lint ✅, build-test ✅, Analyze (go) in progress, [code]smith skipped.
Prior round: round-1 LGTM at 2ee029f (findings F1–F4, all 🟢) covered a 5-file diff. That SHA is unreachable in the current history (branch rewritten), so this round is a full review. Round-1 F3 (test packaging) and F4 (.worktrees/ gitignore churn) no longer apply — the files they referenced were removed by the rewrite; that removal is what F5 flags.

Review Metadata

Reviewers: rev-codex (security) — approve, no findings; rev-claude (correctness/integration) — approve on code, one integration finding (dropped test coverage)
Consensus: approve on code correctness; split on the coverage regression (rev-claude raised it, rev-codex did not opine on it)
Absent reviewers: none

×ばつ0 ×ばつ1 ×ばつ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 requested 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 request_changes — 🔴0 🟡1 🟢2. Reviewed at 729d3bd. Full report: #276 (comment)

@leechenghsiu leechenghsiu mentioned this pull request

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

Merged

3 tasks

leechenghsiu added a commit that referenced this pull request


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

4147c0b

## 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. #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
- [x] `make test`, `make build` pass
- [x] 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](https://claude.com/claude-code)

Labels

None yet

1 participant