fix(cli): return failure exit code for SQL batches

[suibianwanwank]

When a statement fails in a multi-statement sql --batch command, the CLI prints an error but previously exited with status 0. Automation could therefore treat a partially failed deployment as successful.

Set the business-error exit code (1) for both failed SQL results and caught submission/execution exceptions. Continue processing subsequent statements and emitting per-statement results as before; successful statements do not clear the failure exit code. Document the exit behavior in --batch help.

Validation:

• Added six network-boundary regression cases covering -e and -f, all-success batches, failed SQL results, and HTTP errors. They verify later statements still execute and their results are emitted.
• Before the fix, all four failure cases reproduced exit 0 instead of 1.
• 24 tests passed across batch exit codes, SQL error job IDs, split configuration, and USE validation.
• bun typecheck passed in packages/cz-cli.

No production SQL was executed.

[github-actions]

MEDIUM (confidence: high on the code path, asking about intent) — --dry-run has the same hole this PR closes for --batch, and the two now contradict each other in help output.

.option("batch", { … describe: "Batch mode: execute all statements sequentially; exit non-zero if any fails" })
.option("dry-run", { type: "boolean", default: false, describe: "EXPLAIN recognized readonly queries. …" })

The dry-run branch aggregates per-statement outcomes and then hands the whole thing to success():

// sql.ts:661-671
if (r.status === JobStatus.FAILED)
  return { sql: stmt, status: "error", job_id: r.jobId, error: await formatQueryError(…, "EXPLAIN failed") }
…
success({ statements: results, count: statements.length }, { format, rowsKey: "statements" })

success() ends with process.exitCode = EXIT_OK (src/output/index.ts:68), so a dry-run whose EXPLAIN failed prints status: "error" on stdout and exits 0 — exactly the "automation treats a partial failure as success" symptom in the PR description. Because the dry-run branch returns before the batch loop, cz-cli sql --batch --dry-run "…" also still exits 0 while the --batch help text on the line above now promises "exit non-zero if any fails".

Is this deliberately out of scope? If so it's worth saying in the PR body, since --batch and --dry-run are the two multi-statement modes an automation harness reaches for. The parallel fix is one line: set EXIT_BIZ_ERROR when any results entry has status === "error" before calling success() (note the order matters — success() overwrites process.exitCode, so it has to be set after, or success() has to stop asserting EXIT_OK on that path).

[github-actions]

LOW (confidence: high) — the batch branch now does two of error()'s three jobs and skips the third, which leaves the telemetry event for a failed batch without an error code.

} catch (err) {
  process.exitCode = EXIT_BIZ_ERROR

error() (src/output/index.ts:132-135) renders the envelope, sets process.exitCode, and records process.lastError = message. Both new assignments here (this one and line 707) correctly avoid error() — it would print a single top-level error envelope instead of a per-statement line — but they also skip the lastError write, and that value is what both entry points use to label the telemetry event:

• src/run-cli.ts:865-867: track(!process.exitCode, process.exitCode ? lastError ?? \exit_code=${process.exitCode}` : undefined)`
• src/execute.ts:108-116: same lastError ?? \exit_code=${exitCode}`` fallback

So a --batch run that fails a statement now reports success: false, error: "exit_code=1", where a failing single statement reports the real message. Net still better than the pre-PR success: true, but the code/message is already in hand two lines below (classifyExecError, and r.errorCode in the sibling branch) — setting it alongside the exit code in both branches would make the failure legible in telemetry too.

Unrelated to the above, and worth keeping: putting the assignment before the awaited formatQueryError/formatClassifiedError means the exit code survives a throw from the formatter (it calls resolveAccountDisplayName, which does I/O). Deliberate or not, that ordering is the right one.

[github-actions]

Review verdict

A. Upstream invasiveness — no issues found. The diff is two files, both under packages/cz-cli/ (src/commands/sql.ts, new test/sql-batch-exit-code.test.ts). Nothing under packages/opencode, packages/tui, packages/core or packages/schema is touched, so no banner and no UPSTREAM-PATCHES.md INTRUSIVE entry is owed.

B. Clean fix, with one adjacent gap. The core change is the right shape, not a hole drilled around the problem: every other SQL failure path already sets the exit code because it routes through error() / handleFailure() (including the non-batch multi-statement path at sql.ts:729-737), and the --batch branch was the only one writing its own line and therefore the only one that lost it. README.md:121 already documents 0 success / 1 business error / 2 usage error, so this aligns the batch path with an existing contract rather than inventing a new one. No new flag, no fallback, no copy-pasted logic, no dead code, no drive-by edits.

Two inline findings:

• MEDIUM on the --dry-run option line — --dry-run still funnels per-statement status: "error" through success() and exits 0, including --batch --dry-run, which now contradicts the --batch help text one line above.
• LOW on the catch branch — the new assignments skip error()'s lastError write, so a failed batch is tracked as error: "exit_code=1" with no error code.

C. Regression risk. One intended behavioral change, plus the help string:

• cz-cli sql --batch with more than one statement now exits 1 when any statement returns JobStatus.FAILED or throws; previously 0. stdout shape is unchanged (still one JSON line per statement, same keys), so parsers are unaffected — only $? / set -e consumers see the difference. That is the point of the PR and it is documented in --batch's help.
• Single-statement --batch is untouched: statements.length > 1 gates the loop, so one statement still goes through executeSingle → emitResult/handleFailure, which already set the code.
• Existing batch coverage that asserts exit 0 uses only successful statements, so none of it flips: test/sql-readonly.test.ts:90-96 (two stubbed sqlSuccess submits), test/e2e.ts:120, test/e2e-full.ts:58, test/e2e-use.ts:121. The write-preflight batch cases (sql-readonly.test.ts:35-50, :105-115) already expect exit 1 from the preflight, before any submit.
• Help text: nothing asserts the replaced string — semicolon-separated appears nowhere else in the repo, and test/e2e-help/core-cases.ts:21 asserts option names only.
• No change to exported API, config keys, on-disk paths, or command output shape. No test deleted, skipped, or loosened.
• New test coverage: test/sql-batch-exit-code.test.ts covers the touched path directly (failure at index 1, success at index 2, asserting both the later statement's result and exit 1), which is also what pins the "a later success does not clear the exit code" property — that property holds only because the batch branch never calls success(). --batch + --async is not covered, but the batch branch never passes asynchronous to execSqlWithRetry, so that combination executes synchronously and cannot reach the unhandled ExecResult shape.

I did not run the test suite, so nothing above is a claim that tests pass.

[github-actions]

MEDIUM (confidence: medium) — this helper re-implements the body of error(), and makes process.lastError a process-global with two independent writers in two modules.

function markStatementFailure(message: string) {
  process.exitCode = EXIT_BIZ_ERROR
  ;(process as unknown as Record<string, unknown>).lastError = message
}

output/index.ts:134-135 is the same two statements:

process.exitCode = opts?.exitCode ?? EXIT_BIZ_ERROR
;(process as unknown as Record<string, unknown>).lastError = message

lastError is an undeclared slot on process reached only by casting. Its readers are run-cli.ts:865 and execute.ts:108; until now its only writer was error(), so the cast lived once, next to the readers' contract. Writing it from a command module means the shape of that channel is now asserted in two places, and the cast hides any divergence from the type system.

The smaller correct change is to export the pair from output/index.ts — e.g. export function markFailure(message: string, exitCode = EXIT_BIZ_ERROR) — have error() call it, and import it here. That keeps one writer and removes the as unknown as Record<string, unknown> cast from sql.ts.

Worth noting the two existing partial-failure precedents set the exit code only and never touch lastError (status.ts:43, status.ts:81, analytics-agent.ts:1621), so there are now three variants of "mark this run as failed" in the package.

[github-actions]

MEDIUM (confidence: high that the behavior changes; asking about intent) — please confirm the --dry-run exit-code change is intended.

const failed = results.find((r) => r.status === "error")
if (failed) markStatementFailure(String(failed.error))

The PR title, body, and validation notes are all about --batch: "When a statement fails in a multi-statement sql --batch command…", "Document the exit behavior in --batch help", "Added six network-boundary regression cases covering -e and -f". But this hunk also changes sql … --dry-run, a separate code path that returns at line 681 before batch is ever reached. cz-cli sql "SELECT 1; SELECT bad" --dry-run previously exited 0 with statements[1].status === "error" in the payload; it now exits 1. A caller doing cz-cli sql "…" --dry-run && deploy changes behavior.

It looks deliberate (the --dry-run describe string on line 873 was updated and three dry-run cases were added to the new test file), so this is a request to confirm rather than a bug report — but the description undersells the scope, and --dry-run is the flag most likely to be sitting in someone's && chain precisely because it used to be advisory.

Second, smaller point on the same two lines: this works only because it runs after success(), which sets process.exitCode = EXIT_OK unconditionally (output/index.ts:68). Moving it above the success() call on line 678 silently reverts the fix with no test-visible cause at this site. The equivalent code in analytics-agent.ts:1618-1621 carries that constraint in a comment:

// success() resets exitCode to EXIT_OK; override AFTER it so a partial
// failure surfaces as a non-zero exit for scripts.
if (failed > 0) process.exitCode = EXIT_BIZ_ERROR

Per AGENTS.md ("Add comments for non-obvious constraints and surprising behavior"), the same note belongs here.

[github-actions]

LOW (confidence: high) — in a batch, each failure overwrites lastError, so telemetry reports the last failing statement; the dry-run site at line 679 uses results.find(...) and reports the first. Same flag family, opposite answer for "which error did this run hit".

markStatementFailure(line.error.message)

Calling it per-statement also means three mutation sites for one piece of run state. The established shape for this in the package is to count failures in the loop and set the exit code once after it (analytics-agent.ts:1621: if (failed > 0) process.exitCode = EXIT_BIZ_ERROR). Here that would be a const failures: string[] = [] accumulated in both failure branches, then one post-loop if (failures.length > 0) markStatementFailure(failures[0]) — one site, and whichever of first/last you pick becomes an explicit choice that matches dry-run.

Not a correctness bug: the exit code is 1 either way, and the existing tests only assert the exit code, not lastError.

[github-actions]

LOW (confidence: medium) — this newly routes raw engine error text from batch statements into telemetry, untruncated.

;(process as unknown as Record<string, unknown>).lastError = message

message here is formatQueryError(...) / formatClassifiedError(...) output, i.e. the gateway's own error string. execute.ts:108-115 and run-cli.ts:865-867 read lastError whenever exitCode is non-zero and pass it to trackCommand({ error }), which emits it verbatim as the cz_cli.command.error OTel attribute (telemetry.ts:220) with no length cap and no redaction.

Before this PR a batch partial failure exited 0, so telemetry recorded success: true and sent nothing; now it sends the message. That is parity with the single-statement path (handleFailure → error() already does the same), so it is not a new class of exposure — but engine errors frequently echo the offending statement, and .github/claude-review-context.md flags this repo for plaintext credentials having reached telemetry before. A statement like CREATE STORAGE CONNECTION … ACCESS_KEY='…' that fails inside a batch would now put that text in the span attribute.

If that is a concern, the fix is at the one shared writer rather than here (see the markFailure suggestion on line 507): cap the length and strip credential-shaped literals once, and both the single-statement and batch paths get it.

[github-actions]

LOW (confidence: high) — the rewrite drops the only hint in --help about how to pass more than one statement.

.option("batch", { alias: "B", type: "boolean", default: false, describe: "Batch mode: execute all statements sequentially; exit non-zero if any fails" })

Upstream of this change it read "execute multiple semicolon-separated statements sequentially". "all statements" no longer tells a reader where the statement boundary comes from, and nothing else in this command's help or epilogue says it. Adding the exit-code sentence does not require removing that: "Batch mode: execute multiple semicolon-separated statements sequentially; exit non-zero if any fails" keeps both.

One adjacent note while you are in this string, not a request to change code: "execute all statements sequentially" is accurate for the exit code but not for the output shape — --batch with a single statement never reaches the batch branch at line 706, it falls through to executeSingle at line 752 and prints the ordinary single-statement envelope rather than an indexed batch line. That asymmetry is pre-existing and out of scope here.

[github-actions]

Review summary

A. Upstream invasiveness — no issues found

The diff touches two files, both under packages/cz-cli/: src/commands/sql.ts and the new test/sql-batch-exit-code.test.ts. Nothing under packages/opencode, packages/tui, packages/core or packages/schema is modified, so no banner and no new UPSTREAM-PATCHES.md INTRUSIVE entry are required. The cz_change:-style explanatory comments added here are inside the cz layer, which the ledger treats as ordinary comments.

B. Clean fix vs. hole drilled around the problem — no issues found on the substance

This is the right fix, not a workaround. The root cause is structural: process.exitCode is owned by success()/successRows()/error() in src/output/index.ts, and the batch branch deliberately bypasses all three — it writes per-statement lines with a bare process.stdout.write(renderOutput(...)) so each statement renders as its own JSONL record. Nothing on that path had ever claimed the exit code, so it kept whatever success() last set. Setting it at the failure sites, while continuing the loop and still emitting every subsequent statement's result, is the minimal correct change; no new flag, no fallback, no special case, and the failed statements' payloads are unchanged. The new markStatementFailure helper has three call sites, so it is not a preemptive single-use extraction.

Two refactor-shaped observations, both inline rather than blocking:

• line 507 — the helper duplicates the exit-code + process.lastError pair from output/index.ts:134-135, giving that undeclared process-global a second writer in a second module, reached by cast. Exporting the pair from output/index.ts and having error() delegate keeps one writer.
• line 717 — marking per-statement makes lastError the last failure in a batch but the first in dry-run, and gives one piece of run state three mutation sites.

No dead code, no leftover debug output, no copy-paste into a second place, and no unrelated drive-by edits.

C. Regression risk

Enumerated below. I did not run the test suite, so nothing here is a claim that anything passes.

Intended behavior changes

• sql --batch with a multi-statement input where any statement fails: exit 0 → 1. Covered by the four failure cases in the new test/sql-batch-exit-code.test.ts, which also assert that later statements still execute (submitted has 3 entries) and that their results are still emitted. This is the stated purpose of the PR.
• sql --dry-run where any EXPLAIN fails: exit 0 → 1. Covered by the two new dry-run failure cases. Raised separately as a question on lines 679-680 — it is a different code path from --batch, it returns before the batch loop is reached, and the PR description and validation notes mention only --batch. It reads as deliberate (the --dry-run describe string was updated too), but --dry-run is the flag most likely to be sitting in a caller's && chain.
• Telemetry: a batch or dry-run partial failure now reports success: false with the statement's error message instead of success: true. No test covers the telemetry payload. See line 509.

Checked and found not to regress

• Existing --batch tests all use succeeding fixtures, so their exit-0 assertions still hold: test/sql-readonly.test.ts:88-94 (--write --batch, expects exit 0), test/e2e-use.ts:119-121, test/e2e.ts:120, test/e2e-full.ts:58.
• The all-success batch and dry-run cases in the new file pin exit 0, so the happy path is now asserted rather than assumed.
• No output shape change. Both failure branches set the exit code before/after writing, and the line objects are byte-identical to before; test/output-row-projection.test.ts:132-148, which pins the batch record's rendering across formats, is unaffected.
• No exported API, config key, or on-disk path changed. EXIT_BIZ_ERROR was already exported from src/index.ts:9.
• No CLI flag renamed or removed, and no default changed — only two describe strings. Nothing in the repo asserts on either string (e2e-help/core-cases.ts does not), so the help-text edits break no test. One wording regression noted at line 872.
• No tests deleted, skipped, or loosened.
• Callers: markStatementFailure is new and file-local. execute() is exported from src/index.ts:23 but the only in-repo caller is src/bootstrap/forward.ts:9, which forwards the exit code rather than branching on it; no in-repo script, skill, or plugin shells out to sql --batch, so nothing internal consumes the old exit 0.
• No new cross-package dependency edge; the only new import is EXIT_BIZ_ERROR from a sibling module already imported on the same line.
• The new test registers its onFetch handlers inside each test body, which is safe — test/support/fetch-boundary.ts:100 clears handlers in a global afterEach. sql_split defaults to true (sql.ts:70), so the batch cases do split into three statements as the assertions expect, and HTTP 400 is not an auth error so execSqlWithRetry does not re-submit, keeping submitted at exactly 3.

Uncovered paths this change does not reach (pre-existing, listed for completeness, no action implied): --batch --async, where execSqlWithRetry returns an async marker rather than a QueryResult and neither branch at lines 715-726 emits a line or marks a failure; and a USE/SET statement following a failed statement in the same batch, which routes through applyUseStatement → error() and so was already non-zero.