fix(cli): harden the argument and output surface against silent misreports

packages/cz-cli/UPSTREAM-PATCHES.md

@@ -410,6 +410,26 @@ the hooks they depend on still exist in the new upstream.
 
 ---
 
+### 6. `cz-cli serve` flag surface
+
+- **cz files:** `packages/cz-cli/src/bootstrap/runtime.ts` (the `serve` branch:
+  `applyServeLogFlags`, `serveInheritedGlobals`).
+- **Mechanism:** `cz-cli serve` runs its own yargs instance around upstream's
+  `ServeCommand`, so it never passes through upstream's ROOT parser. Two things are
+  re-created there: the root parser's logging flags (`--print-logs`, `--log-level`,
+  `--pure`), wired to the same env vars upstream's root middleware sets
+  (`OPENCODE_PRINT_LOGS`, `OPENCODE_LOG_LEVEL`, `OPENCODE_PURE`), and the cz global
+  flags that run-cli has already consumed off the same argv, declared hidden so
+  `.strict()` can reject a real typo without rejecting a working invocation.
+- **Upstream hooks to re-verify:** `packages/opencode/src/cli/index.ts` still declares
+  those three flags on the root parser and its middleware still reads them from those
+  env var names; `packages/opencode/src/cli/cmd/serve.ts` still takes its network
+  options from `withNetworkOptions`.
+- **Failure mode if the hook moves:** the logging flags silently go back to being
+  accepted and doing nothing (an env var rename), or `serve` starts rejecting a flag
+  it should accept (a new global in cli.ts's `KNOWN_GLOBAL_FLAGS` is covered
+  automatically; a new UPSTREAM root flag is not).
+
 ## Re-baseline procedure (quick)
 
 1. Fast-forward upstream packages to the new opencode version.

packages/cz-cli/src/agent-mcp.ts

@@ -196,10 +196,21 @@ function manifestName(relativePath: string) {
 }
 
 function resolveClickZettaRemote(manifest: ClickZettaRemoteManifest, cliArgs: Partial<CliArgs>): RemoteMcpConfig | undefined {
-  const connection = resolveConnectionConfig({
-    ...cliArgs,
-    profile: manifest.profile ?? cliArgs.profile,
-  })
+  // `manifest.profile` is a field in a config file on disk, not something the caller
+  // typed, so a profile that has since been renamed or deleted must skip THIS entry —
+  // the way a manifest with no usable auth already does — rather than abort the whole
+  // invocation. resolveConnectionConfig rejects an explicitly named missing profile,
+  // and nothing between here and run-cli's injectAgentMcp call would catch it.
+  let connection: ReturnType<typeof resolveConnectionConfig>
+  try {
+    connection = resolveConnectionConfig({
+      ...cliArgs,
+      profile: manifest.profile ?? cliArgs.profile,
+    })
+  } catch (err) {
+    if ((err as { code?: unknown } | null)?.code === "PROFILE_NOT_FOUND") return undefined
+    throw err
+  }
   if (!hasLakehouseAuth(connection) && manifest.enabled !== false) return undefined
   return compactEntry({
     type: "remote",

packages/cz-cli/src/bootstrap/runtime.ts

@@ -7,10 +7,53 @@ import { flushOtel } from "../opencode-plugin/otel/index.js"
 import { flushLangfuse, initLangfuse } from "../langfuse.js"
 import { CLICKZETTA_AGENT_SYSTEM_PROMPT } from "../agent-system-prompt.js"
 import { parseAgentTimeoutMs } from "./runtime-config.js"
+import { KNOWN_GLOBAL_FLAGS } from "../cli.js"
 import { applyBaseOpencodeEnv, applyAgentRuntimeInjection } from "./opencode-injection.js"
 
 let globalHandlersRegistered = false
 
+/**
+ * The cz global flags that reach `cz-cli serve`'s own parser, as hidden no-ops.
+ *
+ * Built from cli.ts's KNOWN_GLOBAL_FLAGS — the single list the top-level parser and
+ * both fail handlers already use — so adding a global there cannot silently start
+ * failing `serve`. `help`/`version` are declared separately above with their real
+ * behavior, and the short aliases are declared as their own entries because they
+ * arrive un-canonicalized on this path.
+ */
+function serveInheritedGlobals(): Record<string, { type: "string" | "boolean"; hidden: true }> {
+  const booleans = new Set(["debug", "d"])
+  const skip = new Set(["help", "h", "version", "v"])
+  const options: Record<string, { type: "string" | "boolean"; hidden: true }> = {}
+  for (const flag of KNOWN_GLOBAL_FLAGS) {
+    if (skip.has(flag)) continue
+    options[flag] = { type: booleans.has(flag) ? "boolean" : "string", hidden: true }
+  }
+  return options
+}
+
+/** The logging flags `cz-cli serve` accepts, mirroring upstream's root parser. */
+export interface ServeLogFlags {
+  "print-logs"?: boolean
+  "log-level"?: string
+  pure?: boolean
+}
+
+/**
+ * cz_change: wire `cz-cli serve`'s logging flags to the env vars opencode reads.
+ *
+ * These three are declared on upstream's ROOT parser (opencode's index.ts), which
+ * `cz-cli serve` never goes through, so before this they were accepted and did
+ * nothing — and could not be rejected either, because turning on .strict() without
+ * declaring them would have failed a documented invocation. Exported so a test can
+ * assert the wiring without starting a server.
+ */
+export function applyServeLogFlags(flags: ServeLogFlags): void {
+  if (flags["print-logs"]) process.env.OPENCODE_PRINT_LOGS = "1"
+  if (flags["log-level"]) process.env.OPENCODE_LOG_LEVEL = String(flags["log-level"])
+  if (flags.pure) process.env.OPENCODE_PURE = "1"
+}
+
 export async function main(args: string[], agentRuntime = false): Promise<number> {
   // cz_change: apply the base opencode env injection (kill upstream auto-updater,
   // disable repo-local project config, telemetry defaults) at the very top of main()
@@ -118,11 +161,35 @@ export async function main(args: string[], agentRuntime = false): Promise<number
     ])
     await yargs(args)
       .scriptName("cz-cli")
+      // cz_change: same invariant as src/cli.ts — yargs' built-in messages must
+      // not localize to the shell's LANG, or a caller parsing them (and
+      // test/robustness.test.ts) sees Chinese on a zh_CN machine.
+      .locale("en")
       .help("help", "show help")
       .alias("help", "h")
       .version("version", "show version number", InstallationVersion)
       .alias("version", "v")
       .command(ServeCommand)
+      // cz_change: these three belong to upstream's ROOT parser (opencode's
+      // index.ts), which `cz-cli serve` never goes through — so they used to be
+      // accepted and do nothing. Declared here, wired to the same env vars
+      // upstream's middleware sets, which is also what lets .strict() below reject
+      // a real typo: `serve --prot 8080` silently started on the default port
+      // (which is 0, i.e. a random one) instead of reporting the flag.
+      .option("print-logs", { type: "boolean", describe: "print logs to stderr" })
+      .option("log-level", { type: "string", choices: ["DEBUG", "INFO", "WARN", "ERROR"], describe: "log level" })
+      .option("pure", { type: "boolean", describe: "run without external plugins" })
+      // cz_change: `serve` is in run-cli.ts's RUNTIME_COMMANDS, so the outer layer has
+      // ALREADY read the cz global flags off this same argv — the connection ones
+      // select the lakehouse the served agent connects as (via ConnectionEnv), and
+      // `--format` is re-inserted after the command word by normalizeCliArgs. They all
+      // still arrive here, so .strict() below would reject invocations that work
+      // today. Declared from the CLI's own list rather than by hand so the two cannot
+      // drift, hidden and unused because the values are consumed before this parser
+      // ever sees them — the same thing runLlm does for `--profile`.
+      .options(serveInheritedGlobals())
+      .middleware((opts) => applyServeLogFlags(opts as ServeLogFlags), true)
+      .strict()
       .demandCommand(1, "")
       .parseAsync()
     return (process.exitCode as number) ?? 0
@@ -291,6 +358,10 @@ export async function main(args: string[], agentRuntime = false): Promise<number
   const cli = yargs(agentArgs)
     .parserConfiguration({ "populate--": true })
     .scriptName("cz-cli agent")
+    // cz_change: pin English, as src/cli.ts does. Without it the agent subtree
+    // answered `agent session list --format csv` with "无效的选项值：…" on a
+    // zh_CN machine while the rest of the CLI stayed English.
+    .locale("en")
     .wrap(100)
     .help("help", "show help")
     .alias("help", "h")

packages/cz-cli/src/cli.ts

@@ -1,9 +1,10 @@
 import yargs from "yargs"
 import { VERSION } from "./version.js"
-import { defaultFormat, outputState, parseOutputArgs, renderOutput } from "./output/index.js"
+import { HandledCliError, defaultFormat, outputState, parseOutputArgs, renderErrorOutput } from "./output/index.js"
 import { withClickZettaProfileOption } from "./clickzetta-profile-option.js"
 import { suggestClosest } from "./suggest.js"
 import { SubcommandHelpShown } from "./subcommand-help.js"
+import { UsageError } from "./usage-error.js"
 
 export interface GlobalArgs {
   profile?: string
@@ -38,25 +39,106 @@ const JSON_ARRAY_OPTIONS = new Set(["--output-tables"])
 export const KNOWN_GLOBAL_FLAGS = ["profile", "p", "jdbc", "pat", "username", "password", "service", "protocol", "instance", "workspace", "schema", "s", "vcluster", "format", "field", "debug", "d", "help", "h", "version", "v", "target", "t"]
 export const KNOWN_TOP_COMMANDS = ["sql", "schema", "table", "workspace", "workspace-param", "status", "auth", "login", "profile", "task", "runs", "attempts", "job", "agent", "serve", "setup", "update", "datasource", "ai-gateway", "analytics-agent", "dqc", "mcp"]
 
+/**
+ * Collapse a repeated scalar option to its last occurrence, leaving options
+ * declared `array: true` alone. See the call site in createCli for why.
+ *
+ * Typed loosely and passed as `never`: yargs' published MiddlewareFunction type
+ * takes only argv, while the runtime also hands in the yargs instance — which is
+ * the only way to learn what the CURRENT subcommand declared as an array.
+ */
+function collapseDuplicateScalars(argv: Record<string, unknown>, instance: unknown): void {
+  const declaredArrays = new Set<string>(readDeclaredArrayKeys(instance))
+  for (const [key, value] of Object.entries(argv)) {
+    // `_` and `--` are yargs' operand lists and `$0` is the script name: all three
+    // are arrays by definition, never a repeated option.
+    if (key === "_" || key === "$0" || key === "--") continue
+    if (Array.isArray(value) && !declaredArrays.has(key) && value.length > 1) {
+      argv[key] = value[value.length - 1]
+    }
+  }
+}
+
+/** yargs' list of keys declared `array: true` on the instance in scope. */
+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 []
+  }
+}
+
+function isNaNValue(value: unknown): boolean {
+  if (typeof value === "number") return Number.isNaN(value)
+  return Array.isArray(value) && value.some((item) => typeof item === "number" && Number.isNaN(item))
+}
+
+/** `pageSize` → `page-size`, so the error names the flag the user typed. */
+function kebab(key: string): string {
+  return key.replace(/[A-Z]/g, (upper) => `-${upper.toLowerCase()}`)
+}
+
+/** Does `text` begin a JSON array/object literal? */
+function startsJson(text: string): boolean {
+  const trimmed = text.trimStart()
+  return trimmed.startsWith("[") || trimmed.startsWith("{")
+}
+
+/**
+ * Is every bracket `text` opened closed again, ending outside a string? Only what
+ * the fragment merging below needs — "is this value still incomplete", not "is this
+ * valid JSON", which JSON.parse decides later in the command.
+ */
+function isClosedJson(text: string): boolean {
+  let depth = 0
+  let inString = false
+  let escaped = false
+  for (const char of text) {
+    if (escaped) { escaped = false; continue }
+    if (inString) {
+      if (char === "\\") escaped = true
+      else if (char === '"') inString = false
+      continue
+    }
+    if (char === '"') inString = true
+    else if (char === "[" || char === "{") depth++
+    else if (char === "]" || char === "}") depth--
+  }
+  return !inString && depth === 0
+}
+
 export function coalesceJsonArrayOptionArgs(args: string[]): string[] {
   const result: string[] = []
   for (let i = 0; i < args.length; i++) {
     const arg = args[i]!
     const isLongForm = JSON_ARRAY_OPTIONS.has(arg)
-    const isEqForm = !isLongForm && [...JSON_ARRAY_OPTIONS].some((name) => arg.startsWith(name + "="))
+    const eqName = isLongForm ? undefined : [...JSON_ARRAY_OPTIONS].find((name) => arg.startsWith(name + "="))
+    if (!isLongForm && !eqName) {
+      result.push(arg)
+      continue
+    }
     // Long form needs a value token that is not itself a flag; otherwise leave it for yargs.
-    if (!isEqForm && (!isLongForm || args[i + 1] === undefined || args[i + 1]!.startsWith("-"))) {
+    if (isLongForm && (args[i + 1] === undefined || args[i + 1]!.startsWith("-"))) {
       result.push(arg)
       continue
     }
-    let value = isLongForm ? args[i + 1]! : arg
+    const prefix = eqName ? `${eqName}=` : ""
+    let value = eqName ? arg.slice(prefix.length) : args[i + 1]!
     let j = isLongForm ? i + 2 : i + 1
-    while (j < args.length && !args[j]!.startsWith("-")) {
+    // Absorb following tokens only while the value is an UNCLOSED JSON literal.
+    // Merging every non-flag token instead ate the command's own positional:
+    // `--output-tables '[{"a": 1}]' mytask` (quotes stripped by the caller, so the
+    // JSON arrives split on its inner space) swallowed `mytask`, and the command
+    // then failed with "Not enough non-option arguments". A value that never
+    // started a JSON literal absorbs nothing at all.
+    while (j < args.length && !args[j]!.startsWith("-") && startsJson(value) && !isClosedJson(value)) {
       value += args[j]!
       j++
     }
     if (isLongForm) result.push(arg, value)
-    else result.push(value)
+    else result.push(prefix + value)
     i = j - 1
   }
   return result
@@ -158,6 +240,38 @@ export function createCli(args: string[]) {
       hidden: true,
       default: false,
     })
+    // Repeating a scalar option is a user slip, and yargs' answer to it is an
+    // ARRAY: `--field a --field b` reached extractField as ["a","b"] and crashed
+    // on field.replace, `--protocol http --protocol https` crashed in
+    // normalizeProtocol, and `--profile p1 --profile p2` silently resolved to no
+    // profile at all. Collapse to last-wins here — the GNU convention — but only
+    // for options NOT declared `array: true`, of which this CLI has 15 (`--set`,
+    // `--variable`, `--header`, …) that must keep collecting. yargs' global
+    // `duplicate-arguments-array: false` cannot make that distinction: it would
+    // reduce those to their last element too.
+    //
+    // 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)
+    // A `type: "number"` option fed a non-number becomes NaN, and nothing else in
+    // the CLI checks: `task cron-preview '0 0 * * *' --count abc` answered "0
+    // upcoming runs" for a valid cron, and paginated commands sent `null` for a
+    // page. NaN can only originate from a number-typed option, so no schema is
+    // needed here — a NaN in argv IS a rejected value. check() reports through
+    // the fail handler, i.e. USAGE_ERROR with exit 2, like any other bad value.
+    .check((argv) => {
+      const invalid = new Set<string>()
+      for (const [key, value] of Object.entries(argv)) {
+        if (key === "_" || key === "$0" || key === "--") continue
+        if (isNaNValue(value)) invalid.add(kebab(key))
+      }
+      if (invalid.size > 0) {
+        const names = [...invalid].map((name) => `--${name}`).join(", ")
+        throw new UsageError(`Invalid number value for: ${names}`)
+      }
+      return true
+    })
     .middleware((argv) => {
       const rawArgs = args.map(a => String(a))
       const hasExplicitFormat = rawArgs.some(
@@ -168,7 +282,11 @@ export function createCli(args: string[]) {
     }, /* applyBeforeValidation */ true)
     .strict()
     .fail((msg, err, failYargs) => {
-      if (err) throw err
+      // Our own validators (see the check() above) report through UsageError so
+      // they get the USAGE_ERROR envelope; anything else is a real exception and
+      // must keep propagating rather than being relabelled as bad usage.
+      if (err && !(err instanceof UsageError)) throw err
+      if (err instanceof UsageError) msg = err.message
       // Defensive net: a group built with raw `.demandCommand()` (no commandGroup
       // fail handler of its own, e.g. mcp / some agent.ts subtrees) bubbles its
       // "Missing subcommand for 'X'" failure straight up here. Resolve it like
@@ -180,6 +298,10 @@ export function createCli(args: string[]) {
         failYargs.showHelp((help: string) => process.stdout.write(help + "\n"))
         throw new SubcommandHelpShown()
       }
+      // A UsageError carries OUR message, already complete. Running it through the
+      // scan below could append "Did you mean '--limit'?" to a message about a
+      // perfectly valid flag, because the scan only looks at the raw tokens.
+      const selfReported = err instanceof UsageError
       const KNOWN_FLAGS = KNOWN_GLOBAL_FLAGS
       const KNOWN_COMMANDS = KNOWN_TOP_COMMANDS
       const knownFlagSet = new Set(KNOWN_FLAGS)
@@ -190,13 +312,15 @@ export function createCli(args: string[]) {
       let badToken: string | undefined
       let suggestion: string | undefined
       let isFlag = false
-      const unknownFlags = args.filter((a) => a.startsWith("-")).map((a) => a.replace(/^-+/, "").split("=")[0]).filter((a) => a && !knownFlagSet.has(a))
+      const unknownFlags = selfReported
+        ? []
+        : args.filter((a) => a.startsWith("-")).map((a) => a.replace(/^-+/, "").split("=")[0]).filter((a) => a && !knownFlagSet.has(a))
       if (unknownFlags.length > 0) {
         isFlag = true
         badToken = unknownFlags[0]
         const hit = suggestClosest(badToken!, KNOWN_FLAGS.filter((f) => f.length > 1))
         if (hit) suggestion = `--${hit}`
-      } else {
+      } else if (!selfReported) {
         const topLevelCmd = args.find((a) => !a.startsWith("-"))
         if (topLevelCmd !== undefined && !knownCommandSet.has(topLevelCmd)) {
           badToken = topLevelCmd
@@ -215,12 +339,18 @@ export function createCli(args: string[]) {
       const outputArgs = parseOutputArgs(args)
       const errorObj: Record<string, unknown> = { code: "USAGE_ERROR", message }
       if (suggestion) errorObj.did_you_mean = suggestion
-      const output = renderOutput({
+      // renderErrorOutput, not renderOutput: a usage error must look like every
+      // other error in the chosen format — `ERROR USAGE_ERROR: …` under
+      // text/csv/table/jsonl, JSON under json/pretty/toon.
+      const output = renderErrorOutput({
         error: errorObj,
         ai_message: aiMessage,
       }, outputArgs.format, outputArgs.field)
       process.stdout.write(output + "\n")
       process.exitCode = 2
-      throw new Error(msg ?? "usage error")
+      // HandledCliError, not a bare Error: the envelope above IS the report, and a
+      // catch further out (runCliWithTracking's last-resort envelope, runLlm's
+      // stderr line) must be able to tell "already reported" from a real exception.
+      throw new HandledCliError("USAGE_ERROR", message)
     })
 }