diff --git a/artifacts/requirements.yaml b/artifacts/requirements.yaml index 830d73c..e40c7ff 100644 --- a/artifacts/requirements.yaml +++ b/artifacts/requirements.yaml @@ -6940,3 +6940,51 @@ artifacts: created-by: ai-assisted model: claude-opus-4-8 timestamp: 2026-06-14T12:00:00Z + + - id: REQ-219 + type: requirement + title: "A clap parse failure under `--format json` must emit a JSON error envelope on stdout, not an empty payload (#500)" + status: implemented + description: | + Finding (#500, hit while implementing #488). rivet's top-level `--project` + / `--schemas` options are NOT clap `global` args, so they parse only + before the subcommand: `rivet --project X validate` works, + `rivet validate --project X` is a parse error. clap writes that error to + stderr and leaves stdout empty — so a consumer running + `rivet validate --format json --project X` as a subprocess got an empty + payload and a cryptic "EOF while parsing a value", with no hint that the + arg *position* was the cause. (The obvious "just add `global = true`" fix + is a trap: `global` debug-asserts against positional subcommands and broke + main once — REQ-209 / #501, reverted #502.) + + Fix (safe, no `global`): parse via `Cli::try_parse()`; on error, when the + raw argv requested JSON (`--format json` / `-f json` in any spelling), + print a one-line JSON envelope `{ "error": , "hint": "pass + --project/--schemas BEFORE the subcommand …" }` to stdout, then still emit + clap's normal human message to stderr and exit with clap's code. + `--help` / `--version` (clap "errors" with exit 0) pass through unchanged; + non-JSON parse errors keep the stderr-only behavior (empty stdout). + + Acceptance: + - `rivet validate --format json --project .` (misplaced `--project`) + exits non-zero and prints a single JSON object on stdout with string + `error` and `hint` fields; the hint mentions putting the option + BEFORE the subcommand. + - `rivet validate --project .` (no JSON) exits non-zero with an EMPTY + stdout (clap message on stderr only). + - `rivet --project . validate --format json` (correct order) still + produces the normal JSON result. + - `rivet --help` / `--version` still print and exit 0. + - `cargo test -p rivet-cli --test cli_commands` passes; + `cargo fmt --check` + `cargo clippy --all-targets -- -D warnings` clean. + tags: [cli, json, dx, error-handling, dogfooding] + fields: + priority: should + category: functional + links: + - type: traces-to + target: REQ-007 + provenance: + created-by: ai-assisted + model: claude-opus-4-8 + timestamp: 2026-06-14T13:30:00Z diff --git a/rivet-cli/src/main.rs b/rivet-cli/src/main.rs index 36b7493..00e8031 100644 --- a/rivet-cli/src/main.rs +++ b/rivet-cli/src/main.rs @@ -1944,8 +1944,83 @@ enum CheckAction { }, } +/// Does the raw argv request JSON output (`--format json` / `-f json` in any +/// of its accepted spellings)? Used only to decide whether a clap *parse* +/// failure should also emit a machine-readable error envelope (#500). +fn argv_requests_json() -> bool { + let mut prev_was_format_flag = false; + for a in std::env::args().skip(1) { + if prev_was_format_flag { + prev_was_format_flag = false; + if a == "json" { + return true; + } + } + match a.as_str() { + "--format" | "-f" => prev_was_format_flag = true, + "--format=json" | "-f=json" | "-fjson" => return true, + _ => {} + } + } + false +} + +/// #500: clap writes parse errors to stderr and leaves stdout empty, so a +/// consumer that ran `rivet validate --format json --project X` (with the +/// top-level `--project` in the wrong position — it is deliberately NOT a +/// clap `global` arg, since `global` debug-asserts against positional +/// subcommands and broke the build once, #501/#502) gets an empty payload and +/// a cryptic "EOF while parsing a value", with no hint about the real cause. +/// When the invocation asked for JSON, also print a one-line JSON error +/// envelope on stdout so machine consumers get a structured, actionable error. +/// `--help`/`--version` (which clap reports as "errors" with exit 0) pass +/// through unchanged. +fn handle_parse_error(err: clap::Error) -> ExitCode { + use clap::error::ErrorKind; + if matches!( + err.kind(), + ErrorKind::DisplayHelp + | ErrorKind::DisplayVersion + | ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand + ) { + // Renders help/version to stdout and exits 0 — never returns. + err.exit(); + } + + if argv_requests_json() { + // Strip ANSI styling clap may have applied, and collapse to one line. + let ansi = regex::Regex::new("\u{1b}\\[[0-9;]*m").ok(); + let raw = err.to_string(); + let cleaned = ansi.map_or_else(|| raw.clone(), |re| re.replace_all(&raw, "").into_owned()); + let summary = cleaned + .lines() + .map(str::trim) + .find(|l| !l.is_empty()) + .unwrap_or("argument parse error") + .trim_start_matches("error:") + .trim() + .to_string(); + let envelope = serde_json::json!({ + "error": summary, + "hint": "rivet's top-level options are not positional-global: pass \ + --project/--schemas BEFORE the subcommand, e.g. \ + `rivet --project `", + }); + if let Ok(s) = serde_json::to_string(&envelope) { + println!("{s}"); + } + } + + // Preserve clap's normal human-facing stderr message and exit code. + let _ = err.print(); + ExitCode::from(u8::try_from(err.exit_code()).unwrap_or(2)) +} + fn main() -> ExitCode { - let cli = Cli::parse(); + let cli = match Cli::try_parse() { + Ok(cli) => cli, + Err(e) => return handle_parse_error(e), + }; let log_level = if cli.quiet { // Below the default `warn` — only hard errors. Suppresses the diff --git a/rivet-cli/tests/cli_commands.rs b/rivet-cli/tests/cli_commands.rs index cd99c10..62d3d78 100644 --- a/rivet-cli/tests/cli_commands.rs +++ b/rivet-cli/tests/cli_commands.rs @@ -6400,6 +6400,52 @@ fn mutating_commands_refuse_on_parse_broken_source() { ); } +/// #500: a clap parse failure (e.g. the non-global `--project` placed AFTER the +/// subcommand) used to leave stdout empty, so a `--format json` consumer got a +/// cryptic "EOF while parsing". Now such invocations also emit a one-line JSON +/// error envelope on stdout, while non-JSON invocations keep clap's stderr-only +/// behavior. +#[test] +fn json_consumers_get_an_error_envelope_on_parse_failure() { + // `--project` after the subcommand is a parse error; with --format json we + // must get a JSON envelope on stdout (not empty), exit non-zero. + let bad = Command::new(rivet_bin()) + .args(["validate", "--format", "json", "--project", "."]) + .output() + .expect("run rivet"); + assert!( + !bad.status.success(), + "a misplaced --project must still fail" + ); + let stdout = String::from_utf8_lossy(&bad.stdout); + let parsed: serde_json::Value = serde_json::from_str(stdout.trim()) + .unwrap_or_else(|e| panic!("stdout must be a JSON envelope, got {stdout:?}: {e}")); + assert!( + parsed.get("error").and_then(|v| v.as_str()).is_some(), + "envelope must carry an 'error' string" + ); + assert!( + parsed + .get("hint") + .and_then(|v| v.as_str()) + .is_some_and(|h| h.contains("BEFORE the subcommand")), + "envelope must hint at the arg-position fix; got: {stdout}" + ); + + // Same parse error WITHOUT a JSON request: stdout stays empty (clap writes + // its message to stderr), preserving the human path. + let bad_text = Command::new(rivet_bin()) + .args(["validate", "--project", "."]) + .output() + .expect("run rivet"); + assert!(!bad_text.status.success()); + assert!( + bad_text.stdout.is_empty(), + "non-JSON parse error must not print an envelope to stdout; got: {}", + String::from_utf8_lossy(&bad_text.stdout) + ); +} + /// #479: next-id must not reissue an ID that is claimed in git history (a /// commit trailer / subject) but absent from the working tree — the /// reverted-but-burned trap (e.g. REQ-209). Git awareness is best-effort and