Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hellozepp merged 2 commits into from
Oct 8, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f49ff94
fix(cli): return failure exit code for SQL batches
Sep 29, 2026
7574edf
fix(cli): fail dry-run when any EXPLAIN fails
Oct 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions packages/cz-cli/src/commands/sql.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { Argv } from "yargs"
import { readFileSync, openSync, readSync, closeSync } from "node:fs"
import { analyzeSql, isReadonlySqlSetting, splitSql, JobStatus, requestRaw, getCurrentUser, type JobID, type QueryResult } from "@clickzetta/sdk"
import type { GlobalArgs } from "../cli.js"
import { success, successRows, error, handledError, parseOutputArgs, renderOutput, renderErrorOutput } from "../output/index.js"
import { success, successRows, error, handledError, parseOutputArgs, renderOutput, renderErrorOutput, EXIT_BIZ_ERROR } from "../output/index.js"
import { maskRows } from "../output/masking.js"
import { logOperation } from "../logger.js"
import { type ExecContext, classifyExecError, execSql, execSqlWithRetry, getExecContext, isQueryResult, validateIdentifier } from "./exec.js"
Expand Down Expand Up @@ -502,6 +502,13 @@ async function resolveAccountDisplayName(ctx: ExecContext) {
}
}

// Per-statement failures are already rendered inline, so error() would print a
// second envelope; set the exit code and telemetry error it would have set.
function markStatementFailure(message: string) {

@github-actions github-actions Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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

@github-actions github-actions Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

}

async function formatQueryError(r: QueryResult, ctx: ExecContext, profileName?: string, fallback = "Query failed") {
return formatBillingError({
code: r.errorCode,
Expand Down Expand Up @@ -669,6 +676,8 @@ async function handler(argv: SqlArgs): Promise<void> {
}
}))
success({ statements: results, count: statements.length }, { format, rowsKey: "statements" })
const failed = results.find((r) => r.status === "error")
if (failed) markStatementFailure(String(failed.error))
Comment on lines +679 to +680

@github-actions github-actions Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

return
}
ctx = await getExecContext(argv)
Expand Down Expand Up @@ -705,6 +714,7 @@ async function handler(argv: SqlArgs): Promise<void> {
const r = await execSqlWithRetry(ctx, stmt, { hints: accumulatedHints, timeoutMs: argv.timeout * 1000, configStatements })
if (isQueryResult(r) && r.status === JobStatus.FAILED) {
const line = { index: i, sql: stmt, error: { code: r.errorCode ?? "SQL_ERROR", message: await formatQueryError(r, ctx, argv.profile) }, time_ms: Date.now() - t0, ...(r.jobId ? { job_id: r.jobId } : {}) }
markStatementFailure(line.error.message)

@github-actions github-actions Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

process.stdout.write(renderOutput(line, format, batchField) + "\n")
logOperation("sql", { sql: stmt, ok: false, errorCode: r.errorCode })
} else if (isQueryResult(r)) {
Expand All @@ -717,6 +727,7 @@ async function handler(argv: SqlArgs): Promise<void> {
} catch (err) {
const { code, message } = classifyExecError(err)
const line = { index: i, sql: stmt, error: { code, message: await formatClassifiedError({ code, message, ctx, profileName: argv.profile }) }, time_ms: Date.now() - t0 }
markStatementFailure(line.error.message)
process.stdout.write(renderOutput(line, format, batchField) + "\n")
logOperation("sql", { sql: stmt, ok: false, errorCode: code })
}
Expand Down Expand Up @@ -858,8 +869,8 @@ export function registerSqlCommand(cli: Argv<GlobalArgs>): void {
.option("header", { type: "boolean", default: true, describe: "Include column names in output. Use --no-header or -N to suppress." })
.option("N", { type: "boolean", hidden: true })
.option("limit", { type: "number", default: 100, describe: "Max rows to return (0 for unlimited)" })
.option("batch", { alias: "B", type: "boolean", default: false, describe: "Batch mode: execute multiple semicolon-separated statements sequentially" })
.option("dry-run", { type: "boolean", default: false, describe: "EXPLAIN recognized readonly queries. Multiple queries require sql_split=true; writes, session commands and unknown syntax are rejected." })
.option("batch", { alias: "B", type: "boolean", default: false, describe: "Batch mode: execute all statements sequentially; exit non-zero if any fails" })

@github-actions github-actions Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

.option("dry-run", { type: "boolean", default: false, describe: "EXPLAIN recognized readonly queries; exit non-zero if any fails. Multiple queries require sql_split=true; writes, session commands and unknown syntax are rejected." })
.epilogue([
"Examples:",
" cz-cli sql \"SELECT * FROM orders LIMIT 10\"",
Expand Down
78 changes: 78 additions & 0 deletions packages/cz-cli/test/sql-batch-exit-code.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import { beforeEach, expect, test } from "bun:test"
import { join } from "node:path"
import { onFetch, requireTestHome, sqlFailure, sqlSuccess, stubStudioContext } from "./support/cz-fixtures.js"

const { execute } = await import("../src/execute.ts")

beforeEach(async () => {
stubStudioContext()
await Bun.write(join(requireTestHome(), ".clickzetta", "profiles.toml"), [
'default_profile = "test"',
"[profiles.test]",
'pat = "pat"',
'service = "uat-api.clickzetta.com"',
'instance = "inst"',
'workspace = "ws0"',
].join("\n"))
})

// Exercise the real command and SDK with only the HTTP boundary substituted.
for (const input of ["execute", "file"] as const) {
for (const failure of ["none", "job", "http"] as const) {
test(`batch ${input}: ${failure} failure preserves results and exit status`, async () => {
const submitted: string[] = []
onFetch({
match: (url) => url.includes("/lh/submitJob"),
respond: (_url, _method, body) => {
const query = (body as { jobDesc: { sqlJob: { query: string[] } } }).jobDesc.sqlJob.query[0]
submitted.push(query)
if (query.includes("SELECT 2") && failure === "job") {
return sqlFailure("CZLH-42000", "Statement failed")
}
if (query.includes("SELECT 2") && failure === "http") {
return new Response("Submission rejected", { status: 400 })
}
return sqlSuccess(["value"], [[1]])
},
})

const sql = "SELECT 1; SELECT 2; SELECT 3;"
const file = join(requireTestHome(), "batch.sql")
if (input === "file") await Bun.write(file, sql)
const result = await execute("sql --batch --sync", input === "file" ? ["-f", file] : ["-e", sql])
const rows = result.output.trim().split("\n").map((line) => JSON.parse(line))

expect(submitted).toHaveLength(3)
expect(rows.map((row) => row.index)).toEqual([0, 1, 2])
expect(rows[0].rows).toEqual([[1]])
expect(rows[2].rows).toEqual([[1]])
expect(result.exitCode).toBe(failure === "none" ? 0 : 1)
if (failure === "none") {
expect(rows.every((row) => !row.error)).toBe(true)
return
}
expect(rows[1].error.code).toBe(failure === "job" ? "CZLH-42000" : "HTTP_400")
})
}
}

for (const failure of ["none", "job", "http"] as const) {
test(`dry-run: ${failure} failure preserves results and exit status`, async () => {
await Bun.write(join(requireTestHome(), ".clickzetta", "czcli.json"), '{"sql_split":true}')
onFetch({
match: (url) => url.includes("/lh/submitJob"),
respond: (_url, _method, body) => {
const query = (body as { jobDesc: { sqlJob: { query: string[] } } }).jobDesc.sqlJob.query[0]
if (query.includes("SELECT 2") && failure === "job") return sqlFailure("CZLH-42000", "Statement failed")
if (query.includes("SELECT 2") && failure === "http") return new Response("Submission rejected", { status: 400 })
return sqlSuccess(["plan"], [["ok"]])
},
})

const result = await execute("sql", ["SELECT 1; SELECT 2", "--dry-run"])
const statements = JSON.parse(result.output).data.statements as { status: string }[]

expect(statements.map((s) => s.status)).toEqual(failure === "none" ? ["ok", "ok"] : ["ok", "error"])
expect(result.exitCode).toBe(failure === "none" ? 0 : 1)
})
}
Loading