fix(cli): harden the argument and output surface against silent misreports - #78
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| .option("log-level", { type: "string", choices: ["DEBUG", "INFO", "WARN", "ERROR"], describe: "log level" }) | ||
| .option("pure", { type: "boolean", describe: "run without external plugins" }) | ||
| .middleware((opts) => applyServeLogFlags(opts as ServeLogFlags), true) | ||
| .strict() |
There was a problem hiding this comment.
HIGH (confidence: high) — .strict() here rejects flags that cz-cli serve currently honors.
.middleware((opts) => applyServeLogFlags(opts as ServeLogFlags), true)
.strict()Declaring the three upstream root flags closes the --prot 8080 hole, but .strict() also closes flags this parser never declared and that the outer layer does act on:
serveis inRUNTIME_COMMANDS(run-cli.ts:155), sorunClirunsconnectionOverridesFromArgs→applyAgentConnectionEnv→ConnectionEnv.pin(profileOverride)for it.--profile/-ptherefore selects which lakehouse the served agent connects as — a real, working invocation today.normalizeCliArgsdoes not strip those tokens fromruntimeArgs:extractGlobalFormatArgs(run-cli.ts:188) pulls out only--format, and then re-inserts it atcommandIndex + 1(run-cli.ts:493-500). So the parser here receives["serve", "--format", "json", "--profile", "prod", …].
Net effect after this change:
cz-cli serve --profile prod → Unknown argument: profile (previously: connected as prod)
cz-cli serve --format json → Unknown argument: format (previously: accepted, ignored)
cz-cli serve --debug → Unknown argument: debug
The --profile one is a functional regression, not just a stricter error. The same treatment the three logging flags got would fix it: declare profile/p, format, field, debug/d here (hidden, no-op — the outer layer has already consumed them) before turning on .strict(). runLlm already does exactly that for --profile, and its cz_change: comment at commands/agent-llm.ts says why.
I don't see a test for serve with any global flag — test/e2e-routing.ts isn't in this diff, so I can't tell whether it covers serve --profile.
| function isProfileCreatingInvocation(command: string, args: string[]): boolean { | ||
| if (command === "setup" || command === "login") return true | ||
| if (command !== "auth") return false | ||
| const authIndex = args.indexOf("auth") | ||
| return args.slice(authIndex + 1).find((token) => !token.startsWith("-")) === "login" | ||
| } |
There was a problem hiding this comment.
HIGH (confidence: high) — the exemption list is too narrow, and a stale CZ_PROFILE now locks the user out of the very command the error message tells them to run.
function isProfileCreatingInvocation(command: string, args: string[]): boolean {
if (command === "setup" || command === "login") return true
if (command !== "auth") return false
const authIndex = args.indexOf("auth")
return args.slice(authIndex + 1).find((token) => !token.startsWith("-")) === "login"
}The guard at line 750 fires on profileOverrideFromArgs(...) ?? ConnectionEnv.profileName(), i.e. on CZ_PROFILE too (connection/env.ts:88). It runs before command dispatch, so it applies to every command that isn't --help, setup, login, or auth login. With CZ_PROFILE pointing at a profile the user has since deleted:
CZ_PROFILE=ghost cz-cli profile list
→ PROFILE_NOT_FOUND: … next_step: "cz-cli profile list"
That remedy is itself blocked. The same applies to the rest of the recovery surface — profile create <name> (commands/profile.ts:258, a profile-creating command that the exemption misses), profile use, profile delete, auth list, auth logout — plus commands with no connection at all: cz-cli --version, cz-cli update.
Root cause is that the guard is keyed on "did the caller name a profile" rather than "does this command need to connect". PROFILE_REQUIRED_COMMANDS (line 30) already encodes the latter and is consulted 60 lines further down. Gating on that set, or exempting the whole profile and auth groups, would keep the fix for the reported case (--profile <typo> on sql/status) without the lockout.
Separately: emitProfileNotFound exits 1 while emitUsageError exits 2 for the neighbouring class of "you typed something wrong". Worth confirming 1 is deliberate (it does match error()'s EXIT_BIZ_ERROR).
| success(sessions, { | ||
| format, | ||
| extra: { active_session: activeSession ?? null, active_profile: activeProfile ?? null }, | ||
| }) |
There was a problem hiding this comment.
HIGH (confidence: high) — this is the one call site that breaks its JSON contract, and rowsKey (added in this same PR) exists precisely to avoid that.
success(sessions, {
format,
extra: { active_session: activeSession ?? null, active_profile: activeProfile ?? null },
})--format json output changes shape:
// before
{ "data": { "sessions": [...], "active_session": "uat", "active_profile": "uat_0" } }
// after (success() adds `count` for an array payload — output/index.ts:59)
{ "data": [...], "count": 2, "active_session": "uat", "active_profile": "uat_0" }So jq '.data.sessions' and --field sessions both stop working. test/auth-list-format.test.ts:1287 pins the new shape, so this is deliberate — but the PR description says "JSON output is unchanged by it" and the "Behavior changes worth a look" list doesn't mention it, and nothing in the repo records it (no known-changes.ts entry, unlike the --limit/--no-limit note for #70).
The smaller correct change is the mechanism the other seven call sites in this PR got:
success({ sessions, active_session: activeSession ?? null, active_profile: activeProfile ?? null },
{ format, rowsKey: "sessions" })projectRows step 3 projects data.sessions for the row formats and leaves json/pretty/toon untouched — the row-format fix you're after, with no JSON break and no --field sessions regression. Worth checking why this one diverged from the pattern.
| if (Array.isArray(payload.columns) && Array.isArray(payload.rows)) { | ||
| const columns = payload.columns.filter((column): column is string => typeof column === "string") | ||
| const rows = (payload.rows as unknown[]).map((row) => (Array.isArray(row) ? row : [row])) | ||
| return { columns, rows } | ||
| } |
There was a problem hiding this comment.
MEDIUM-HIGH (confidence: high) — branch 1 now claims sql --batch payloads, which are not bare SQL envelopes, and strips the per-statement identity that batch mode exists to provide.
if (Array.isArray(payload.columns) && Array.isArray(payload.rows)) {commands/sql.ts:661-662 (not in this diff, so easy to miss) builds:
const line = { index: i, sql: stmt, columns, rows, count: rows.length, time_ms: …, job_id? }
process.stdout.write(renderOutput(line, format, batchField) + "\n")Before: emitAsTable/emitAsCsv looked only at obj.data, found nothing, and fell through to formatPretty(line); emitAsText fell through to formatJson(line). So the whole record — index, sql, time_ms, job_id — was in the output. emitAsJsonl did project via obj.rows, so jsonl is the one format that is unchanged.
After, --batch --format table|csv|text emits only the result grid:
index/sql/time_ms/job_idare gone, so a consumer can no longer tell which statement a grid belongs to;- consecutive statements' grids concatenate with no delimiter — for csv, a second header line mid-stream;
- a DDL statement (
columns: []) hitsemitRowFormat'sprojection.columns.length === 0early return, so line 662 writes"" + "\n"— a bare blank line where a JSON record used to be.
The error branch at sql.ts:656 is unaffected (no columns, so it takes NON_TABULAR_FALLBACK), which makes the inconsistency worse: within one --batch --format csv run, failures stay JSON blobs and successes become anonymous CSV.
Either narrow branch 1 (e.g. require the absence of an index/sql sibling, or key it on the successRows envelope rather than on any object carrying columns+rows), or update sql.ts's batch path to emit deliberately. I don't see a test covering sql --batch under a row format.
|
|
||
| logOperation("profile list-workspaces", { ok: true }) | ||
| success({ region: target, instances: targetInstances, workspaces_by_instance: workspacesByInstance }, { format }) | ||
| success({ region: target, instances: targetInstances, workspaces_by_instance: workspacesByInstance }, { format, rowsKey: "instances" }) |
There was a problem hiding this comment.
MEDIUM (confidence: high) — rowsKey: "instances" makes list-workspaces stop reporting workspaces under every row format.
success({ region: target, instances: targetInstances, workspaces_by_instance: workspacesByInstance }, { format, rowsKey: "instances" })workspaces_by_instance (built at line 714-722) is the command's actual answer. Because it is a sibling of the projected list, projectRows drops it: per the docstring, "scalar siblings of a projected list are not part of the table". Before this change, emitAsCsv/emitAsTable/emitAsText rendered data as a single row with columns region | instances | workspaces_by_instance, so the workspaces were present (JSON-encoded in a cell). Now:
cz-cli profile list-workspaces --region cn-shanghai --format csv
→ a table of instance metadata, zero workspaces
instances is also the less interesting half — profile list-instances presumably already covers it.
If a flat row shape is wanted here, the list to project is the workspaces, which means reshaping the payload (one row per instance × workspace) rather than pointing rowsKey at the sibling. If that's more than this PR wants to take on, dropping rowsKey leaves the old one-row rendering intact — workspaces_by_instance is a nested object, so the step-4 inference won't fire and nothing changes for this command.
| const requestedProfile = cliArgs.profile ?? ConnectionEnv.profileName() | ||
| if (requestedProfile && !readProfileEntry(requestedProfile)) { | ||
| throw new InterfaceError( | ||
| `Profile '${requestedProfile}' not found in ~/.clickzetta/profiles.toml. Run \`cz-cli profile list\` to see the configured profiles.`, | ||
| { code: "PROFILE_NOT_FOUND" }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: high) — the new guard derives the profile name differently from line 29, so the default_profile half of the same bug is still silent; and the throw is new for programmatic callers that don't catch it.
const profileName = cliArgs.profile ?? Profile.current()
…
const requestedProfile = cliArgs.profile ?? ConnectionEnv.profileName()
if (requestedProfile && !readProfileEntry(requestedProfile)) {Two things:
1. The two derivations disagree, which is what line 24-28's comment warns against. Profile.current() is CZ_PROFILE falling back to profiles.toml's default_profile; ConnectionEnv.profileName() is only the CZ_PROFILE half (env.ts:88). So a profiles.toml whose default_profile = "old_prod" points at a [profiles.*] table that has since been deleted still walks straight past the guard, getProfileConfig returns undefined, every field falls back to env/defaults, and the user gets "Authentication required. Run cz-cli auth login" — the exact misreport this block was added to fix, reached by the other route. If the narrowness is deliberate (a stale default_profile is arguably the tool's own bookkeeping error rather than a user typo, and reporting it would need a different message), a one-line note saying so would keep the next reader from "fixing" it into Profile.current().
2. New unhandled throw for non-CLI callers. resolveConnectionConfig is called from opencode-plugin/tui-quota-data.ts:290 and :677, opencode-plugin/gateway-prompt.ts:144, agent-mcp.ts:199, commands/exec.ts:50, commands/studio-context.ts:24/66/135, commands/job-performance.ts:31, and is re-exported from src/index.ts:5. applyAgentConnectionEnv (run-cli.ts:706-711) explicitly catches PROFILE_NOT_FOUND; none of the others do. On the TUI path in particular, a stale CZ_PROFILE inherited into the session now throws inside the quota sidebar's data fetch instead of rendering degraded. Worth a pass over those call sites, or checking whether they sit inside an existing boundary.
| return { columns, rows: records.map((record) => columns.map((column) => record[column])), items } | ||
| } | ||
| return formatPretty(payload) | ||
| return { columns: items.length > 0 ? [scalarColumn] : [], rows: items.map((value) => [value]), items } |
There was a problem hiding this comment.
MEDIUM (confidence: high) — --format text now renders scalar-list cells through formatFlatCell, which is a different rendering than the old text path.
return { columns: items.length > 0 ? [scalarColumn] : [], rows: items.map((value) => [value]), items }Old emitAsText for an array of scalars:
return data.map((v) => (v === null || v === undefined ? "" : String(v))).join("\n")New path is formatText([scalarColumn], rows) → formatFlatCell (output/formatter.ts:454), so for the same input:
null/undefined→NULL(was the empty string)- a string that
shouldQuoteFlatStringmatches (formatter.ts:472) → JSON-quoted:""for empty,"123"for a numeric-looking name,"true","null", anything with leading/trailing whitespace, and anything containing",\, tab or newline
That reaches every command whose payload is a plain list of strings/numbers — and, newly, the rowsKey sites in this PR whose list is scalar rather than record: schema.ts:78 (rowsKey: "tables" → table names) and task.ts:4205 (rowsKey: "next_runs" → timestamps). A shell loop doing cz-cli schema describe x --format text | while read t now gets "123abc"-style quoting for names that look numeric, and NULL lines where it used to get blanks.
Arguably more correct (it round-trips, and it matches what successRows already does for text — output/index.ts:103), so this may be exactly what you want. But it is a change to --format text's cell grammar that isn't in the "Behavior changes worth a look" list, and test/output-row-projection.test.ts asserts on record lists rather than scalar ones as far as I can see. Worth either pinning it with a scalar-list-with-null case or calling it out.
| const listKeys = Object.keys(data).filter((key) => { | ||
| const value = data[key] | ||
| return Array.isArray(value) && value.length > 0 && value.every(isRecordValue) | ||
| }) | ||
| if (listKeys.length === 1) { | ||
| const listKey = listKeys[0] | ||
| const siblingsAreScalar = Object.keys(data).every((key) => key === listKey || isScalarCell(data[key])) | ||
| if (siblingsAreScalar) return projectArray(data[listKey] as unknown[], listKey) | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: medium-high) — step 4 is opt-out-less inference applied to every command in the CLI, including ones this PR never looked at.
const listKeys = Object.keys(data).filter((key) => {
const value = data[key]
return Array.isArray(value) && value.length > 0 && value.every(isRecordValue)
})
if (listKeys.length === 1) {Steps 1-3 are all triggered by something the caller did (successRows, a bare-array data, an explicit rowsKey). Step 4 fires on payload shape, so any success({ …scalars, oneList: [{…}] }) anywhere in the tree silently changes its table/csv/text/jsonl rendering from "one row of the object's fields" to "a table of oneList, scalars discarded" — with no diff line and no test in this PR to notice it.
Concretely, datasource.ts:582 in this diff:
success({ datasource: ds.name, ds_type: dsType, checks: result.checks, ready: result.ok },
{ format, rowsKey: "checks", aiMessage: aiMsg })ready — the verdict the command exists to deliver — is a scalar sibling, so check-cdc --format csv now returns the individual checks and no overall pass/fail. Note that step 4 would have projected checks here even without rowsKey, so the explicit key isn't what causes it; the inference is.
Two things that would make this safer without giving up the auth list-class fix:
- Make step 4 opt-in (a
rowsKeyor a boolean) and only enable it where a maintainer has looked at the payload. TherowsKeyplumbing is already in place for that. - If it stays inferred, sweep the
success(call sites with a single-array-plus-scalars shape and confirm each one reads correctly.grep -rn "success({" packages/cz-cli/src/commandsis the list.
Not asserting a bug — the inference is well documented and the narrowness conditions are thought through. But "which commands changed output" isn't answerable from this diff, and that's the part worth pinning down before merge.
| * (cli.ts, command-group.ts) render usage errors themselves rather than through | ||
| * error(); they must not diverge from it. | ||
| */ | ||
| export function renderErrorOutput(payload: unknown, format?: string, field?: string): string { |
There was a problem hiding this comment.
LOW-MEDIUM (confidence: high) — one error envelope still bypasses this and keeps the old JSON-under---format text shape.
export function renderErrorOutput(payload: unknown, format?: string, field?: string): string {The docstring's goal is "a --format text consumer reads one shape for every failure", and cli.ts:345 / command-group.ts:101 / error() all route through here now. commands/sql.ts:593 does not:
const sigintHandler = () => {
const payload: Record<string, unknown> = { error: { code: "ABORTED", message: "Execution interrupted by user." } }
if (currentJobId) payload.job_id = currentJobId
process.stdout.write(renderOutput(payload, format, parseOutputArgs(process.argv.slice(2)).field) + "\n")So Ctrl-C during cz-cli sql … --format text still emits {"error":{"code":"ABORTED",…}} while every other failure in that same format now emits ERROR ABORTED: …. Switching it to renderErrorOutput is a one-word change and makes the invariant actually hold; the job_id sibling is dropped by the row-format rendering either way (it's already dropped today, since renderOutput falls through to formatJson for an error payload).
Worth also confirming there are no other direct renderOutput calls on { error: … } payloads — grep -rn "renderOutput(" packages/cz-cli/src is short enough to eyeball.
| const claimsNext = next !== undefined && (!next.startsWith("-") || secretValues.has(i + 1)) | ||
| if (claimsNext) { | ||
| args[key] = isSensitiveKey(key) || isSensitiveValue(next!) ? "<redacted>" : next! | ||
| args[key] = isSensitiveKey(key) || isSensitiveValue(next!) ? "<redacted>" : redactSql(next!) |
There was a problem hiding this comment.
MEDIUM (confidence: high) — this masks the leak rather than closing it; the schema the comment says doesn't exist is available here.
const claimsNext = next !== undefined && (!next.startsWith("-") || secretValues.has(i + 1))
if (claimsNext) {
args[key] = isSensitiveKey(key) || isSensitiveValue(next!) ? "<redacted>" : redactSql(next!)redactSql (logger.ts:13) only rewrites quoted literals that look like a phone number, an ID card, an email, or sit after a sensitive column name. So for the reported case:
cz-cli sql --debug "select * from customers where region = 'apac'"
→ args["debug"] = "select * from customers where region = 'apac'" (unchanged — nothing matched)
The full statement — table names, column names, predicates — still leaves the machine under a boolean flag's key. Only PII-shaped literals are covered, and that's a subset of what the docstring implies ("recorded the whole statement — and unlike the local log, telemetry leaves the machine").
The root cause is claimsNext: a value-less flag cannot be distinguished from a value-taking one by shape, and this file treats that as unfixable. But the CLI does know which flags are booleans — cli.ts declares them (debug/d, format_explicit, plus per-command write, async, batch, no-limit, stdin, with-schema, truncate, …), and cli.ts:63's new readDeclaredKeys(instance, kind) already reads getOptions() lists for exactly this kind of question. A BOOLEAN_FLAGS set here — even just the global ones, which is where the collision actually happens since they can precede a positional — would stop the claim at the source and leave the statement in _positional, where redactSql was already applied and where analytics expects it.
Keeping redactSql as defence in depth is fine. The concern is that with it in place the claimsNext defect looks addressed when the main body of a statement still goes out. If a boolean-flag list is out of scope for this PR, it'd be worth narrowing the docstring so the next reader doesn't read this as solved.
| if (process.exitCode) process.exit(process.exitCode as number) | ||
| process.stderr.write(`Error: ${err instanceof Error ? err.message : String(err)}\n`) |
There was a problem hiding this comment.
LOW (confidence: medium) — this silences genuine exceptions whenever an exit code happens to already be set.
if (process.exitCode) process.exit(process.exitCode as number)
process.stderr.write(`Error: ${err instanceof Error ? err.message : String(err)}\n`)The intent — don't double-report what commandGroup's fail handler already wrote — is right. But the condition is "is process.exitCode non-zero", not "did the fail handler write an envelope for this error". error() (output/index.ts:134) and handledError() also set exitCode, so a subcommand that reports one failure via error() and then throws something unrelated (a network exception in a later step, a bug) exits with no message on either stream. That's the same failure mode run-cli.ts's new last-resort envelope was added to eliminate, just on the agent llm parser instead of the main one.
isHandledCliError(err) / an explicit sentinel would target it precisely, the way runCliWithTracking now does:
if (err instanceof SubcommandHelpShown || isHandledCliError(err)) process.exit(process.exitCode as number ?? 0)Also worth noting both new process.exit() calls here skip telemetry flushing — runLlm already ended in process.exit(0)/(1) so this isn't new behavior for this function, but it does add two more early-exit paths to a file that has them.
| success( | ||
| { connected: false, error: reason, cli_version: VERSION, time_ms: Date.now() - t0 }, | ||
| { format }, | ||
| ) |
There was a problem hiding this comment.
LOW (confidence: high) — please confirm the intent: the new branch's payload omits workspace/schema entirely rather than nulling them.
success(
{ connected: false, error: reason, cli_version: VERSION, time_ms: Date.now() - t0 },
{ format },
)The success branch below emits { connected, workspace, schema, cli_version, time_ms }. This branch drops two keys. For the FAILED-probe case that means:
before: {"data":{"connected":true,"workspace":null,"schema":null,…}} --field workspace → "null"
after: {"data":{"connected":false,"error":"…",…}} --field workspace → "" (extractField returns undefined)
connected: false is clearly the right answer, and the PR description does call out this case. The key omission is the part that isn't mentioned: a consumer doing .data.workspace gets undefined rather than null, which JSON-schema-style validation or a typed client would treat differently. Mirroring the catch branch is consistent, so this may well be deliberate — but adding workspace: null, schema: null here would keep one payload shape across all three outcomes for the cost of two tokens.
Related, on the exit code (process.exitCode = EXIT_BIZ_ERROR at line 41 and 75): this is a scripting contract change for anyone running cz-cli status --format json | jq .connected and relying on exit 0 to mean "the command ran". It's disclosed as change #1 in the PR body and test/status-exit-code.test.ts pins it, so no objection — just flagging that the known-changes.ts note in the description covers only the --limit/--no-limit case for #70, not this.
| // positional picks them up — `cz-cli sql -- "select 1"` lost the statement and | ||
| // answered "No SQL statements found." Collected under `argv["--"]` instead, the | ||
| // same way the agent-runtime parser already does it (bootstrap/runtime.ts). | ||
| .parserConfiguration({ "populate--": true }) |
There was a problem hiding this comment.
LOW (confidence: medium) — populate-- is set on the root, so it applies to every command in the tree, not just sql.
.parserConfiguration({ "populate--": true })The sql motivation is sound and commands/sql.ts:241-242 consumes argv["--"]. The part worth confirming is the blast radius: with populate--: false (the old default) yargs appends post--- tokens to _, where a declared positional can still pick them up; with it on, they go to argv["--"] and nothing else looks at them. sql is the only command in this PR that reads argv["--"] — I grepped and found no argv._ consumers in packages/cz-cli/src, so nothing breaks that way, but any command with a required positional now fails on the ---separated form:
cz-cli profile create -- myname → "Not enough non-option arguments" (previously bound `myname`)
Obscure invocation, and arguably GNU-correct that -- no longer feeds positionals. Raising it only so the choice is deliberate rather than a side effect of the sql fix — a per-command parserConfiguration on sql would have kept the change scoped, if that's preferable.
Second, smaller thing on the .check() below (line 269): it iterates Object.entries(argv) and skips _ and $0 but not --. Harmless today since argv["--"] holds raw strings and isNaNValue only inspects numbers inside arrays, but it's the one key that's now guaranteed present on every parse and is worth skipping alongside the other two for the same reason collapseDuplicateScalars skips it (line 55).
Review verdictA. Upstream invasiveness — no issues foundAll 23 changed files are under Two changes reach into upstream behavior from the cz side and both do it through a hook rather than an edit, correctly: B. Clean fix vs. hole drilled around the problem — mostly clean, three exceptionsThe central refactor is the right shape: four near-copies of the row-projection logic collapsed into one The exceptions are inline:
Also noted at No dead code, commented-out code, leftover debug logging, or unrelated drive-by edits. The 120 new tests are named after the misreports they prevent, which is the right convention here. C. Regression risk — issues foundBehavior this PR can change, with test coverage where I could find it:
No tests were deleted, skipped, or had assertions loosened. On existing callers of the modified functions: I grepped I could not run the test suite, so I am not asserting anything about the reported 1071 pass / 0 fail — the coverage column reflects reading the tests, not executing them. |
| const requestedProfile = profileOverrideFromArgs(normalized.args) ?? ConnectionEnv.profileName() | ||
| const invocationConnects = | ||
| PROFILE_REQUIRED_COMMANDS.has(normalized.command) || normalized.shouldDelegateToAgentRuntime |
There was a problem hiding this comment.
Behaviour change — please confirm intent. MEDIUM, confidence medium.
const requestedProfile = profileOverrideFromArgs(normalized.args) ?? ConnectionEnv.profileName()
const invocationConnects =
PROFILE_REQUIRED_COMMANDS.has(normalized.command) || normalized.shouldDelegateToAgentRuntimeinvocationConnects includes shouldDelegateToAgentRuntime (agent / run / llm / serve), and that path has never been behind the NO_PROFILE gate — requiresProfile at run-cli.ts:809 is keyed on PROFILE_REQUIRED_COMMANDS alone. So for the agent path this gate is the first thing to require that a named profile resolve to a profiles.toml entry.
readProfileEntry returns undefined both for "this name is absent" and for "there is no profiles.toml at all" (connection/profile-store.ts:145-152), and ConnectionEnv.profileName() is just CZ_PROFILE (connection/env.ts:88-91).
Failure scenario: a CI container with no profiles.toml, exporting CZ_PROFILE=ci as a label alongside CZ_INSTANCE / CZ_WORKSPACE / CZ_PAT, runs cz-cli agent run "…". Today the env credentials are used and it works; after this change it exits 1 with PROFILE_NOT_FOUND and next_step: cz-cli profile list, which will list nothing. Same for cz-cli serve and cz-cli llm ….
The recovery-path reasoning in the comment above is sound for profile list / auth list / --version, and resolveConnectionConfig's docstring is explicit that CZ_PROFILE is checked "here" rather than in the library — so I think this is deliberate for the cz command tree. What I could not tell is whether extending it to the agent path was intended, given that path resolves credentials from CZ_* without needing a profile entry. If env-only invocation is supported, gating on readProfileEntry alone will break it; gating on "named profile absent and no usable env credentials" would not.
| success( | ||
| { total: targets.length, succeeded, failed, skipped, results }, | ||
| { format, timeMs: Date.now() - t0 }, | ||
| { format, rowsKey: "results", timeMs: Date.now() - t0 }, |
There was a problem hiding this comment.
Behaviour change — please confirm intent. MEDIUM, confidence high.
success(
{ total: targets.length, succeeded, failed, skipped, results },
{ format, rowsKey: "results", timeMs: Date.now() - t0 },
)projectRows drops the scalar siblings of a projected list (output/index.ts:343-364), so under --format table|csv|text|jsonl this payload now renders only the per-target results rows: total, succeeded, failed and skipped disappear from the output. Previously all five were the columns of a single row, with results as a JSON cell. JSON/pretty/toon are unchanged.
That is the exact hazard cited two files over as the reason not to declare a rowsKey:
commands/datasource.ts:581— "NorowsKey:readyis the verdict this command exists to deliver, and a projected list drops its scalar siblings."
Here failed / skipped play the same role — they are the verdict of a partial batch. A script reading --format csv to find out how many targets failed now has to count rows instead, and skipped has no per-row representation at all to count.
Worth noting the exit code still reports partial failure (the process.exitCode override right below), so this may well be an acceptable trade. Just flagging that the reasoning applied to datasource check and profile list-workspaces points the other way for this one. No test covers the row-format output of this command.
| if (!next) continue | ||
| if (takesNoValue(key)) continue | ||
| if (!isSensitiveKey(key) && !isSensitiveValue(next)) continue | ||
| secretValues.add(i + 1) |
There was a problem hiding this comment.
MEDIUM — confidence medium (real code path, contrived-but-possible trigger).
if (!next) continue
if (takesNoValue(key)) continue
if (!isSensitiveKey(key) && !isSensitiveValue(next)) continue
secretValues.add(i + 1)The new skip sits above the isSensitiveValue(next) test, which weakens the guarantee that test's own docstring states — "Values that are credentials whatever flag carries them" (telemetry.ts:34-51).
Failure scenario: cz-cli … --yes Cookie=<session> (or any KEY=VALUE whose key is in SENSITIVE_KEYS, standing after a value-less flag). Before, index i+1 was added to secretValues and therefore filtered out of positional at :130, so the token never reached telemetry. Now it is skipped, lands in positional, and reaches args["_positional"] at :133. redactSql does not save it: it only rewrites single-quoted literals (logger.ts:8, RE_QUOTED = /'([^']*)'/g), so a bare Cookie=abc123 passes through verbatim to OTel.
Moving the skip below the sensitivity checks keeps both properties: a value-less flag still does not claim its neighbour in the flag map (that decision is made independently at :154), while a credential-shaped token is still excluded from _positional. Concretely, swap the two lines:
| if (!next) continue | |
| if (takesNoValue(key)) continue | |
| if (!isSensitiveKey(key) && !isSensitiveValue(next)) continue | |
| secretValues.add(i + 1) | |
| if (!isSensitiveKey(key) && !isSensitiveValue(next)) continue | |
| secretValues.add(i + 1) |
(i.e. drop the takesNoValue early-continue here; claimsNext at :154 already prevents the flag from recording the value.)
| } | ||
|
|
||
| /** yargs' declared-key lists (`array`, `number`, …) for the instance in scope. */ | ||
| function readDeclaredKeys(instance: unknown, kind: "array" | "number"): string[] { |
There was a problem hiding this comment.
LOW — confidence high. The "number" half of this helper is never called.
function readDeclaredKeys(instance: unknown, kind: "array" | "number"): string[] {readDeclaredKeys has exactly one call site — readDeclaredKeys(instance, "array") at :51 — and nothing in src/ or test/ passes "number". The NaN validation below uses isNaNValue instead, on the sound reasoning that "NaN can only originate from a number-typed option, so no schema is needed here."
Narrowing the parameter to "array" (or inlining it) removes a branch that reads as if the number check consults yargs' declarations when it does not.
| if (err && !(err instanceof UsageError)) throw err | ||
| if (err instanceof UsageError) msg = err.message |
There was a problem hiding this comment.
LOW — confidence medium. Routing our own UsageError through this handler also routes it through the "did you mean" scan further down, which can append a suggestion for a flag that is perfectly valid.
if (err && !(err instanceof UsageError)) throw err
if (err instanceof UsageError) msg = err.messageAt :311 the handler computes unknownFlags by filtering the raw args against knownFlagSet, which is KNOWN_GLOBAL_FLAGS only — so every subcommand flag counts as "unknown" there. That was harmless while msg always came from yargs (the flag really was unknown), but the new check() at :262-275 now arrives here with msg = "Invalid number value for: --count" while --count is a legitimate flag of task cron-preview. badToken becomes "count", and if suggestClosest finds a global within maxDistanceFor (suggest.ts:52-56) the reported message becomes Invalid number value for: --count. Did you mean '--format'?, with errorObj.did_you_mean set — pointing at an unrelated flag for an error that has nothing to do with spelling.
I checked the reachable cases and none actually trip today: timeout gets maxDistance 2, count/limit/page get 2 or 1, and no global flag is that close — which is why parameter-hardening.test.ts:152-155 can assert the bare ERROR USAGE_ERROR: Invalid number value for: --timeout. So this is latent rather than live; a future number flag closer to a global name, or a rename in KNOWN_GLOBAL_FLAGS, would surface it.
Gating the suggestion block on err === undefined — i.e. only when yargs itself reported an unrecognised token — would keep it from firing on our own validators.
| const code = (err as { code?: unknown } | null)?.code | ||
| const payload = { | ||
| error: { | ||
| code: typeof code === "string" && code.length > 0 ? code : "INTERNAL_ERROR", | ||
| message, |
There was a problem hiding this comment.
LOW — confidence high. This promotes arbitrary runtime code properties into the CLI's curated error.code vocabulary.
const code = (err as { code?: unknown } | null)?.code
const payload = {
error: {
code: typeof code === "string" && code.length > 0 ? code : "INTERNAL_ERROR",Every other producer in this CLI emits a code from a fixed set (USAGE_ERROR, NO_PROFILE, PROFILE_NOT_FOUND, FILE_READ_ERROR, CONNECTION_ERROR, …). Here any exception carrying a string code supplies it directly, so Node errno values reach the machine-readable contract: an unhandled fs failure reports "code": "ENOENT", a socket failure "ECONNREFUSED", a DNS failure "ENOTFOUND". A consumer switching on error.code now has an open-ended vocabulary to handle.
This is the last-resort path, so an imperfect code beats a stack trace either way — and the sql --file case that motivated the envelope was separately given a real name (FILE_READ_ERROR in commands/sql.ts:257-262). Worth considering whether an unnamed exception should just be INTERNAL_ERROR with the errno left in message, keeping code closed.
| function projectRows(payload: unknown, rowsKey?: string): RowProjection | undefined { | ||
| if (!isRecordValue(payload)) return undefined | ||
|
|
||
| if (isTabularEnvelope(payload)) { |
There was a problem hiding this comment.
LOW — confidence medium-high. I could not find a production path that reaches this branch; it looks reachable only from the new tests.
if (isTabularEnvelope(payload)) {The docstring says step 1 is "a bare columns + rows envelope — already a table (successRows)". But successRows never routes a row format through renderOutput:
- with no
field,:99-108handlestable/csv/text/jsonlitself viaformatTable/formatCsv/formatText/formatJsonl, and onlyjson/pretty/toonfall through torenderOutput— where the switch never callsemitRowFormat; - with a
field,renderOutput's guard at:153always returns at:157or:160(extracted value, or""when not found), so theswitchis never reached at all.
I grepped every direct renderOutput caller (run-cli.ts:323,333, main.ts:14, commands/autoupdate.ts:47, commands/analytics-agent.ts:725, commands/update.ts:80, commands/sql.ts:662,668,674) and none passes a payload with top-level columns + rows — the sql --batch lines carry index/sql alongside, which isTabularEnvelope deliberately rejects. output-row-projection.test.ts:152,156,277-287 exercise it by calling renderOutput directly.
Not necessarily worth removing — it is a reasonable guard if successRows' fast path is ever folded into emitRowFormat, which would be the actual simplification here (one row-shape decision, as the module docstring argues for). Flagging so the "already a table (successRows)" comment does not read as describing a live path.
One consequence if it ever does go live: TABULAR_ENVELOPE_KEYS whitelists ai_message but not job_id, which commands/sql.ts:553 attaches to essentially every query result via extra. Two otherwise identical sql invocations would then project differently depending on whether a job ID came back.
| // An empty list is empty output, not a JSON envelope leaking into a row format. | ||
| if (projection.columns.length === 0) return "" |
There was a problem hiding this comment.
LOW — confidence high. Regression note on --format csv: an empty list now produces zero bytes, so the header row disappears too.
// An empty list is empty output, not a JSON envelope leaking into a row format.
if (projection.columns.length === 0) return ""Replacing the leaked JSON envelope is clearly right. But for CSV the two failure modes are not symmetrical: a consumer doing csv.DictReader or tail -n +2 over cz-cli … --format csv gets an empty stream rather than a header with no data rows, and empty output is indistinguishable from a command that produced nothing at all.
Note the asymmetry with the columns + rows envelope, which does keep its header when there are no rows — output-row-projection.test.ts:286-289 pins "zero rows still print the header for table and csv". The difference is unavoidable here (an empty [] carries no column names to print), so this may simply be the best available answer; output-row-projection.test.ts:246-249 pins the current behaviour deliberately.
Raising it only so the CSV consequence is a conscious choice rather than a side effect of unifying the four emitters.
|
Review summary A. Upstream invasiveness — no issues found All 23 changed files are under One suggestion, not a finding: B. Clean fix, or a hole drilled around the problem? Mostly the clean fix. Collapsing four near-identical Two smaller items, both filed inline: And one place where the same fix reached only one of two identical handlers: C. Regression risk Inline findings, most severe first:
Behavioural changes I checked and found covered, listed so the blast radius is on the record:
No tests were deleted, skipped, or loosened — the one replaced assertion ( I did not run anything, so nothing here is a claim that the suite passes. |
| // cz_change: `serve` is in run-cli.ts's RUNTIME_COMMANDS, so the outer layer | ||
| // has ALREADY read these off the same argv — `--profile` selects the lakehouse | ||
| // the served agent connects as (via ConnectionEnv.pin), and `--format` is | ||
| // re-inserted after the command word by normalizeCliArgs. They still arrive | ||
| // here, so .strict() below would reject an invocation that works today. | ||
| // Declared hidden and unused: the flags are consumed upstream of this parser, | ||
| // exactly as runLlm declares them for the same reason. | ||
| .option("profile", { type: "string", alias: "p", hidden: true }) | ||
| .option("format", { type: "string", hidden: true }) | ||
| .option("field", { type: "string", hidden: true }) | ||
| .option("debug", { type: "boolean", alias: "d", hidden: true }) | ||
| .middleware((opts) => applyServeLogFlags(opts as ServeLogFlags), true) | ||
| .strict() |
There was a problem hiding this comment.
HIGH — confidence high. The pass-through list is incomplete, so .strict() now rejects serve invocations that work today.
.option("profile", { type: "string", alias: "p", hidden: true })
.option("format", { type: "string", hidden: true })
.option("field", { type: "string", hidden: true })
.option("debug", { type: "boolean", alias: "d", hidden: true })
...
.strict()The reasoning in the comment is right — serve is in RUNTIME_COMMANDS, so the outer layer reads connection flags off this same argv — but --profile is not the only one it reads. connectionOverridesFromArgs (run-cli.ts:532-548) maps thirteen names, and it is called with agentPath = normalized.shouldDelegateToAgentRuntime (run-cli.ts:760), which is true for serve because RUNTIME_COMMANDS contains it (run-cli.ts:155, :496). AGENT_CONTESTED_FLAGS (run-cli.ts:528) removes only s/username/password/u/m/c/f/n from that scan.
So these are still read and expanded into CZ_* by applyAgentConnectionEnv for serve, but are not declared here and are now rejected by .strict():
--jdbc, --pat, --service, --protocol, --instance, --workspace, --schema, --vcluster
Failure scenario: cz-cli serve --workspace ws1 previously pinned CZ_WORKSPACE=ws1 (this parser ignored the flag) and started the server. Now yargs fails with Unknown argument: workspace and no server starts. Same for cz-cli serve --pat czt_… --instance abc. --username/--password/--target were also accepted-and-ignored before and are now rejected, though those never reached the scanner on this path.
The new tests cover --profile and --format (parameter-hardening.test.ts:628-637) but no other connection flag, so nothing catches this.
Smallest fix: declare the same set the scanner reads, e.g. derive both from one exported list so they cannot drift again — the drift is what this comment is already trying to prevent for four of the thirteen.
| // (boolean via --no-limit, number on sql) are declared BOTH ways, so they stay | ||
| // out — treating them as value-less would drop a real value, and for --header | ||
| // that value can be `Authorization=Bearer …`. | ||
| "N", "all", "a", "allow-timeout", "async", "auto-lineage", "batch", "B", "browser", |
There was a problem hiding this comment.
MEDIUM — confidence high. "a" is declared both ways, so by this block's own rule it should stay out of the list.
// `--header` (boolean on sql, KEY=VALUE on profile create) and `--limit`
// (boolean via --no-limit, number on sql) are declared BOTH ways, so they stay
// out — treating them as value-less would drop a real value
"N", "all", "a", "allow-timeout", ...-a is the alias of the boolean --all on agent session list (commands/agent.ts:90) and the alias of --client, a type: "string", array: true option on mcp init (commands/mcp.ts:653-658) — whose own .example() at mcp.ts:668 is cz-cli mcp init -a claude -a codex.
Failure scenario: cz-cli mcp init -a claude -a codex now records args.a = "true" and pushes claude codex into _positional (positional becomes ["mcp","init","claude","codex"]), instead of args.a = "claude". The recorded dimension for which clients were configured is lost and the values land in the free-text positional field. Not a credential leak — --client values are client names — but it is the same "drop a real value" outcome the comment cites as the reason to exclude --header and --limit.
Dropping "a" from the set restores the old behavior for mcp init (its neighbour is recorded) while --all's long form stays covered by "all", which is still in the list.
b2eec93 to
62e2ab1
Compare
| const requestedProfile = cliArgs.profile | ||
| if (requestedProfile && !readProfileEntry(requestedProfile)) { | ||
| throw new InterfaceError( | ||
| `Profile '${requestedProfile}' not found in ~/.clickzetta/profiles.toml. Run \`cz-cli profile list\` to see the configured profiles.`, | ||
| { code: "PROFILE_NOT_FOUND" }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: high) — this throw reaches agent-mcp.ts, one of the four callers the design note says should degrade rather than throw.
const requestedProfile = cliArgs.profile
if (requestedProfile && !readProfileEntry(requestedProfile)) {
throw new InterfaceError(The guard is keyed on "the caller passed profile explicitly", and the new test's docstring lists the callers that must not be affected:
A stale
CZ_PROFILEis deliberately NOT rejected here. […] this function is also reached by callers that merely read a config — the TUI quota sidebar, gateway-prompt, agent-mcp, studio-context — where an inherited value that no longer resolves should degrade, not throw.
But agent-mcp.ts:198 does pass it explicitly, from a file on disk rather than from the invocation:
function resolveClickZettaRemote(manifest: ClickZettaRemoteManifest, cliArgs: Partial<CliArgs>): RemoteMcpConfig | undefined {
const connection = resolveConnectionConfig({
...cliArgs,
profile: manifest.profile ?? cliArgs.profile,
})manifest.profile is a user-written field in an MCP manifest (agent-mcp.ts:104 parses it), and nothing on the path catches: discoverAgentMcp (agent-mcp.ts:222) → injectAgentMcp (:255) → run-cli.ts:807 await injectAgentMcp(agentConnectionOverrides), all uncaught. The boundary guard in runCli can't cover it either — it only inspects --profile/CZ_PROFILE, never the manifest.
Failure scenario: a user has ~/.clickzetta/.mcp/lakehouse/config.json with "profile": "staging", then renames or deletes that profile. Before this PR the entry fell back to env/defaults, hasLakehouseAuth returned false, and the entry was skipped (agent-mcp.ts:203). Now cz-cli agent run "…" aborts with PROFILE_NOT_FOUND — the new last-resort envelope in runCliWithTracking catches it, so it exits 1 with an error envelope, but the agent never starts, over a stale field in an unrelated MCP manifest.
The smaller correct change is to keep the throw for the connection path and have resolveClickZettaRemote treat an unresolvable manifest.profile the same way it already treats a manifest with no usable auth — return undefined and skip the entry. The same code === "PROFILE_NOT_FOUND" check applyAgentConnectionEnv uses would do it.
No test covers manifest.profile naming a nonexistent profile.
| let resolved: ReturnType<typeof resolveConnectionConfig> | ||
| try { | ||
| resolved = resolveConnectionConfig(overrides) | ||
| } catch (err) { | ||
| if ((err as { code?: unknown } | null)?.code === "PROFILE_NOT_FOUND") return overrides | ||
| throw err | ||
| } |
There was a problem hiding this comment.
MEDIUM (confidence: medium-high) — on the agent path this catch both hides the failure and skips the work the function exists to do.
let resolved: ReturnType<typeof resolveConnectionConfig>
try {
resolved = resolveConnectionConfig(overrides)
} catch (err) {
if ((err as { code?: unknown } | null)?.code === "PROFILE_NOT_FOUND") return overrides
throw err
}The comment justifies this by saying the boundary guard in runCli "has already decided" whether a missing profile is an error. But that guard is gated on PROFILE_REQUIRED_COMMANDS (run-cli.ts:30-43), which contains none of agent, run, serve, llm, mcp. So for cz-cli agent run --profile typo:
- the boundary guard at
:750does not fire (invocationConnectsis false), - this catch returns early, so
ConnectionEnv.applyUser/ConnectionEnv.applyat:703-704never run, :764-765then pinstypoviaConnectionEnv.pin(profileOverride).
Net effect: the agent starts with a pinned profile name that does not exist and no connection env applied, and nothing is reported on either stream. Before this PR the same invocation resolved with env/default fallbacks and applied them. So the PR's stated fix — "--profile <typo> reported NO_CREDENTIALS. Now PROFILE_NOT_FOUND" — does not hold on the agent path; it went from a wrong message to no message.
Two options, either of which seems smaller than the current shape:
- add the agent-delegating commands to the boundary guard (the
--helpand profile-creating exemptions already there would carry over), so the throw is genuinely pre-decided and this catch becomes reachable only for the exempt cases; or - report here rather than returning silently.
I don't see a test for agent run --profile <nonexistent>; parameter-hardening.test.ts:179 and :213 both go through status, which is in PROFILE_REQUIRED_COMMANDS.
| // mentioned. Same rule as the cz command tree (see parseRegisteredCommands): a | ||
| // syntactically invalid invocation is reported before any onboarding gate, and in | ||
| // the same envelope as every other usage error. | ||
| if (normalized.shouldDelegateToAgentRuntime && parseAgentTimeoutMs(normalized.args) === null) { |
There was a problem hiding this comment.
LOW (confidence: high) — this gate is keyed on shouldDelegateToAgentRuntime, which is broader than the set of commands that actually have a --timeout.
if (normalized.shouldDelegateToAgentRuntime && parseAgentTimeoutMs(normalized.args) === null) {shouldDelegateToAgentRuntime is true for every member of RUNTIME_COMMANDS (run-cli.ts:155), i.e. run, llm and serve. serve has no --timeout — its options come from upstream's withNetworkOptions (port, hostname, mdns, mdns-domain, cors) plus the three logging flags this PR adds.
Failure scenario: cz-cli serve --timeout abc now answers
--timeout must be a positive number of seconds. Pass the LLM first-byte timeout in seconds, e.g. --timeout 150.
with exit 2, instead of the "Unknown argument: timeout" that the newly-added .strict() on the serve parser would otherwise produce. Advice about an LLM first-byte timeout is wrong for serve, and it also means --timeout is silently accepted on serve whenever the value happens to parse (serve --timeout 150 slips past .strict() here… actually it reaches the serve parser and is rejected there, so the two answers disagree depending only on whether the value is numeric).
Narrowing the condition to the commands that declare the flag — run, and bare agent/agent run — would keep the fix and drop the false positive. isAgentSessionEntry computed just above at :766-771 is close to that set already.
| // renderErrorOutput, like every other failure: under a row format this is | ||
| // `ERROR ABORTED: …` rather than a JSON blob (see its docstring). | ||
| process.stdout.write(renderErrorOutput(payload, format, parseOutputArgs(process.argv.slice(2)).field) + "\n") |
There was a problem hiding this comment.
LOW (confidence: high) — switching this handler to renderErrorOutput drops job_id from the Ctrl-C report under row formats.
process.stdout.write(renderErrorOutput(payload, format, parseOutputArgs(process.argv.slice(2)).field) + "\n")payload is built two lines above as { error: {...} } plus payload.job_id = currentJobId. But renderErrorOutput (output/index.ts:190-199) reduces the whole envelope to one line built from error.code and error.message alone; every sibling key is discarded. So under --format text|csv|table|jsonl the output becomes
ERROR ABORTED: Execution interrupted by user.
and job_id — set precisely so the caller can look the interrupted job up afterwards — is gone. Under --format json it survives, so the loss is format-dependent.
Failure scenario: a script runs cz-cli sql "<long query>" --async --format text, the user Ctrl-Cs, and the wrapper that used to parse the job id out of stdout to poll or cancel the job no longer has one.
Consistency with the other error paths is a reasonable goal, but job_id is payload the row form has no slot for. Either append it to the line, or keep the JSON envelope for this one handler and say why.
I don't see a test covering the SIGINT path under a row format.
| // No rowsKey: `workspaces_by_instance` is what list-workspaces answers, and | ||
| // projecting `instances` would drop it from every row format. Reshaping to | ||
| // one row per instance x workspace is the real fix and is out of scope here. | ||
| success({ region: target, instances: targetInstances, workspaces_by_instance: workspacesByInstance }, { format }) |
There was a problem hiding this comment.
LOW (confidence: high) — this is the one place the rowsKey design leaves the original misreport in place, by its own admission.
// No rowsKey: `workspaces_by_instance` is what list-workspaces answers, and
// projecting `instances` would drop it from every row format. Reshaping to
// one row per instance x workspace is the real fix and is out of scope here.
success({ region: target, instances: targetInstances, workspaces_by_instance: workspacesByInstance }, { format })The defect this PR fixes is "a list wrapped in an object renders as one row with the list JSON-encoded into a cell." Making the projection opt-in via rowsKey is a defensible call — the alternative (inferring it) was tried and demonstrably changed commands nobody had looked at, which the datasource check note explains well. But the consequence is that the fix is a per-caller annotation rather than a fix to the shared behavior, and profile list-workspaces keeps the broken output: --format text still emits a single row whose workspaces_by_instance cell is a JSON blob, and --format csv a single-row CSV with an embedded JSON object.
Worth deciding explicitly whether that is shipped-as-known or should be finished in this PR, since it's the same failure the PR title says it's hardening against. If it stays, the NOT-covered list in the PR description is the right place to name it, alongside the analytics-agent/datasource check cases where "one row carrying the verdict" is genuinely the right answer — those two are a different judgement (deliberate) from this one (deferred), and the identical comment shape makes them read alike.
| function readDeclaredArrayKeys(instance: unknown): string[] { | ||
| try { | ||
| const options = (instance as { getOptions?: () => Record<string, unknown> } | undefined)?.getOptions?.() | ||
| const keys = options?.array | ||
| return Array.isArray(keys) ? keys.filter((key): key is string => typeof key === "string") : [] | ||
| } catch { | ||
| return [] | ||
| } | ||
| } |
There was a problem hiding this comment.
LOW (confidence: high) — this catch turns "I could not read yargs' internals" into "no option is repeatable", which silently discards user input rather than failing.
function readDeclaredArrayKeys(instance: unknown): string[] {
try {
const options = (instance as { getOptions?: () => Record<string, unknown> } | undefined)?.getOptions?.()
const keys = options?.array
return Array.isArray(keys) ? keys.filter((key): key is string => typeof key === "string") : []
} catch {
return []
}
}Both failure modes — a throw, and getOptions() no longer exposing array — return [], and [] is indistinguishable from "this subcommand declares no array options." collapseDuplicateScalars then collapses every repeated option to last-wins, including the 15 the comment at the call site says must keep collecting.
Failure scenario: a yargs bump renames or restructures getOptions().array. cz-cli profile create p --header A=1 --header B=2 then stores only B=2, with no error anywhere — the user's first header is dropped. Nothing about the invocation looks wrong.
parameter-hardening.test.ts:119-130 does cover this (it asserts both headers survive), so the regression would be caught in CI rather than in the field — which is why this is LOW rather than higher. Still, given the whole function is reading an undocumented internal, having it distinguish "no array options" from "could not tell" and skip the collapse entirely in the latter case would fail safe instead of silently lossy.
| // Runs before validation so `choices` sees the scalar, and takes the yargs | ||
| // instance from the middleware's 2nd argument, which in a subcommand reports | ||
| // that subcommand's own declarations. | ||
| .middleware(collapseDuplicateScalars as never, /* applyBeforeValidation */ true) |
There was a problem hiding this comment.
LOW (confidence: high) — both new validators are registered on createCli only, so the agent subtree keeps the two defects this PR fixes.
.middleware(collapseDuplicateScalars as never, /* applyBeforeValidation */ true)createCli builds the cz command tree. The agent path never reaches it: run-cli.ts:806-808 delegates to bootstrap/runtime.ts's own yargs, and agent llm runs on a third parser inside runLlm (commands/agent-llm.ts:857). Neither gets this middleware or the .check() for NaN below it.
So after this PR:
cz-cli sql --field a --field b→ last-wins (fixed)cz-cli agent session list --field a --field b→ still an arraycz-cli agent llm add x --provider openai --api-key k1 --api-key k2→ still an array, and--api-keyreaching the store as["k1","k2"]is the same shape that crashedextractField/normalizeProtocolon the cz sidecz-cli agent llm models x --limit abc→ stillNaNif any number option exists there
That's a scoping choice rather than an error, and the PR description already says "Not covered: opencode's own parser surface on the agent path." Worth confirming that the cz-declared options on those two cz-owned parsers (not upstream's) are meant to be out of scope too — runtime.ts and runLlm are cz files, and both already got .locale("en") in this PR for exactly the "the agent subtree diverged from the rest of the CLI" reason. The same two lines would extend these fixes there.
| function projectArray(items: unknown[], scalarColumn: string): RowProjection { | ||
| if (items.length > 0 && items.every(isRecordValue)) { | ||
| const records = items as Record<string, unknown>[] | ||
| const columns = unionColumns(records) | ||
| return { columns, rows: records.map((record) => columns.map((column) => record[column])), items } | ||
| } | ||
| return formatPretty(payload) | ||
| return { columns: items.length > 0 ? [scalarColumn] : [], rows: items.map((value) => [value]), items } |
There was a problem hiding this comment.
LOW (confidence: medium) — tightening the record test from "first element" to "every element" changes how a heterogeneous list renders.
function projectArray(items: unknown[], scalarColumn: string): RowProjection {
if (items.length > 0 && items.every(isRecordValue)) {All four old emitters keyed off data[0] alone (typeof data[0] === "object" && data[0] !== null && !Array.isArray(data[0])). every is the stricter and generally better test — it's what makes the scalar path safe — but it flips the output for a list whose first element is a record and whose later elements are not.
Failure scenario: an API page comes back as [{id: 1, name: "a"}, null] (a null entry, or one element that is itself an array). Before: a two-column id/name table with an empty row. After: a single column named tasks/sessions/value whose first cell is the whole record JSON-encoded — i.e. exactly the "list JSON-encoded into a cell" shape this PR set out to remove, just applied to the row instead of the list.
formatFlatCell (output/formatter.ts:454-459) JSON-stringifies objects so nothing crashes and nothing prints [object Object]; the output is just less useful than before for that input.
Whether it's worth handling depends on whether any command's list can legitimately contain a null or non-record element — I didn't find one, hence medium confidence and LOW severity. If you want to be safe, items.some(isRecordValue) with unionColumns over just the record elements would keep the table and put the odd element in an empty row, which is closer to the old behavior without giving up the union-columns fix. output-row-projection.test.ts covers uniform record lists and uniform scalar lists; I don't see a mixed case.
| const tables = tableRows.map((row, index) => getShowTableName(row, tableRecords[index] ?? {})) | ||
| logOperation("schema describe", { sql: infoSql, ok: true, timeMs: Date.now() - t0 }) | ||
| success({ name, type: schemaType, table_count: tables.length, tables }, { format, timeMs: Date.now() - t0 }) | ||
| success({ name, type: schemaType, table_count: tables.length, tables }, { format, rowsKey: "tables", timeMs: Date.now() - t0 }) |
There was a problem hiding this comment.
MEDIUM (confidence: high) — asking the author to confirm intent: this changes schema describe's row-format output shape and drops three fields, which is the exact trade declined at two other call sites in the same PR.
success({ name, type: schemaType, table_count: tables.length, tables }, { format, rowsKey: "tables", timeMs: Date.now() - t0 })Before: schema describe demo --format csv emitted one row with columns name,type,table_count,tables.
After: N rows, one column, tables — name, type and table_count are gone from text/table/csv/jsonl. projectRows' own docstring states this: "Scalar siblings of a projected list (active_profile, table_count) are not part of the table."
That's a defensible reading of what schema describe is for, and --format json is unchanged. What makes it worth a look is that the two other commands touched in this PR got the opposite call, for a reason that applies here too:
datasource.ts:581— "No rowsKey:readyis the verdict this command exists to deliver, and a projected list drops its scalar siblings."analytics-agent.ts:1117— "No rowsKey: total/succeeded/failed/skipped are the summary this command reports."
table_count reads like the same kind of fact: the answer to "how big is this schema", derivable from the row count in some formats but not in text (where an empty list now prints nothing at all, so zero tables and a failed projection look identical).
Two questions:
- Is dropping
name/type/table_countfrom row formats intended forschema describe, or wastableschosen because it's the biggest field rather than because it's the answer? - If intended, is any consumer parsing
schema describe --format csv|texttoday?auth list,task cdc list,task cdc tables,task cron-previewandsql --dry-runget the same treatment in this PR — the PR description lists the usage-error andsql-priority changes under "behavior changes worth a look" but not these five output-shape changes, and they seem at least as likely to be parsed by a script.
auth-list-format.test.ts pins the new auth list shape; I don't see equivalent coverage for schema describe.
|
Review summary Reviewed against A. Upstream invasiveness — no issues found No file under The new The B. Clean fix, or a hole drilled around the problem? Mostly the clean fix. Two changes are the right shape and worth saying so. Collapsing the four near-identical Three things to look at, all inline:
No dead code, no leftover debug logging, no unrelated drive-by edits. The C. Regression risk Output shape, which scripts may parse (no
Exit codes:
Semantics:
Tests: none deleted or skipped. The one removed test ( Telemetry: I checked |
…ports Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
62e2ab1 to
24f6e71
Compare
2 participants
Two audits of the CLI's argument and output surface, and the fixes for what they found. Every item is a case where the CLI answered wrongly or unparseably instead of reporting the problem.
Output
text/table/csv/jsonl) now share one row-shape projection instead of four near-copies.auth list --format textprinted the whole session list as JSON inside a single cell; a command whose list should be the table now declares it with the newrowsKeyoption, which only the row formats read — JSON output is unchanged, so.data.sessionsand--field sessionskeep working. Nothing is inferred from payload shape: an earlier revision projected any object holding one array-of-records, which costdatasource checkthereadyverdict it exists to deliver.ERROR <code>: <message>under row formats, like every other error. The two onboarding gates (NO_PROFILE,NO_LLM_CONFIGURED) deliberately keep their structured payload — pinned by a test.jsonlhad no single-object path.Arguments
--field a --field bcrashed,--protocol http --protocol httpscrashed,--profile p1 --profile p2silently resolved to no profile. Now last-wins, leavingarray: trueoptions collecting.type: "number"options accepted junk asNaN(--count abcanswered "0 upcoming runs" for a valid cron). Now aUSAGE_ERROR.--profile <typo>reportedNO_CREDENTIALS("run auth login"). NowPROFILE_NOT_FOUND, with--helpand the profile-creating commands exempt.--workspace "") overwrote the profile and then complained the field was missing. Empty now means absent.sql --file <missing>). Now a named error, plus a last-resort envelope so nothing reaches the entry point unreported;--debugstill shows the stack.sqlread--filebefore the positional statement, contradicting its own--help;--operands never reached it.serveswallowed unknown flags and started anyway (default port is 0, i.e. random). Its three pass-through logging flags are now declared and wired, so.strict()is possible.agent llmexited 1 withError: subcommand help shownfor its own help, and double-reported bad flags.agent run --timeout abcwas masked by the LLM gate.LANG, breaking the English invariantrobustness.test.tssupport use clause #6 pins for the rest of the CLI.--output-tablesJSON value absorbed the command's own positional.redactSqlthe local SQL log uses.Row-format output that changes shape
schema describe,task cdc list,task cdc tables,task cron-preview,sql --dry-runandauth listdeclarerowsKey, so undertext/table/csv/jsonlthey go from one metadata row to N rows and drop the scalar siblings (name/type/table_counton schema describe, the echoedtask_id,cron,count).--format jsonis untouched in all six, and--fieldstill reaches the dropped fields.datasource check,profile list-workspacesand the analytics batch commands deliberately do NOT declare it, because there the scalar sibling (ready,workspaces_by_instance, the totals) is the answer.Behavior changes worth a look
statusexits non-zero when it cannot connect, and reportsconnected: falsewhen a probe query fails (it used to claimconnected: truewith null fields). Payload shape unchanged.--format text|csv|table|jsonlare now oneERROR …row rather than JSON.sqlpositional now wins over--file, as documented.Verified
bun test1118 pass / 0 fail,test/e2e-routing.ts23/23,test/e2e-help.ts105/105,tsgo --noEmitclean — all run on this branch's exact content. 169 of those tests are new (parameter-hardening,output-row-projection,auth-list-format,status-exit-code, plus additions toconnection-configandtelemetry), each named after the misreport it prevents.Three rounds of automated review findings are folded in, including two regressions this PR had introduced:
.strict()onserverejected the twelve cz global flags the outer layer reads off the same argv, and the profile guard — keyed on "was a profile named" rather than on where the name came from — locked a staleCZ_PROFILEout ofprofile list,profile createand even--version. The guard now separates the two sources: a name the caller TYPED is a typo wherever it appears (including on the agent path, where it previously produced no message at all), while an INHERITEDCZ_PROFILEis only fatal for a command that connects. Aprofilefield in an MCP manifest on disk skips that entry rather than aborting the run.packages/cz-cli/UPSTREAM-PATCHES.mdgains a HOOK entry for theserveflag wiring.Not covered: opencode's own parser surface on the agent path, server-side handling of previously-sent
nullpages, andboot.ts's auto-update branch.Note for #70: last-wins makes
sql --limit 1 --no-limitresolve to0instead of the array[1, 0], so that history-regression combination needs aknown-changes.tsentry on that branch.🤖 Generated with Claude Code