diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dbddf744..e36d35e9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -57,6 +57,9 @@ jobs: - uses: dtolnay/rust-toolchain@stable + - name: Run Windows CLI argument error tests + run: cargo test -p bsk --test cli_usage_errors --test cli_parse --locked + - name: Run Windows named-pipe tests run: cargo test -p bsk --lib --locked daemon::ipc::windows::tests diff --git a/CHANGELOG.md b/CHANGELOG.md index ed7af1bf..b201e01a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). Starting from 0.2.0, CLI / Extension / DSH Plugin share the same version number. +## [Unreleased] + +### Fixed + +- CLI: argument parsing failures now honor `--json`, returning an + `invalid_params` error on stdout with `data.reason: "cli_parse_error"` + instead of leaving stdout empty. A misplaced `--session` / `--tab-id` now + explains that it belongs after a supporting subcommand, replacing the + misleading `--version` suggestion and usage line + ([#387](https://github.com/Tencent/BrowserSkill/issues/387)). + ## [0.3.3] - 2026-10-14 ### Changed diff --git a/crates/bsk-cli/README.md b/crates/bsk-cli/README.md index 6f7725e6..57ac967c 100644 --- a/crates/bsk-cli/README.md +++ b/crates/bsk-cli/README.md @@ -11,6 +11,28 @@ export PATH="${BSK_INSTALL_DIR:-$HOME/.local/bin}:$PATH" Documentation: [../../README.md](../../README.md) · [../../docs/architecture.md](../../docs/architecture.md) +## Flags and errors + +`--json`, `--quiet`, and `--verbose` are global flags and may appear before or +after the subcommand. `--session` and `--tab-id` belong to the commands that +support them; place them after the concrete subcommand: + +```sh +bsk --json click --session --tab-id --selector '#submit' +bsk tab list --session --json +``` + +With `--json`, argument parsing failures also write a JSON error to stdout, +including when parsing stops before reaching the flag. The error has the usual +`code`, `message`, `hint`, `exit_code`, and `data` fields, with +`code: "invalid_params"`, `data.reason: "cli_parse_error"`, and exit code `1`. +`message` is a one-line summary such as +`unexpected argument '--session' found`; `data.details` holds the full parser +text, including tips and usage. This applies to redirected stdout as well. +Without `--json`, usage errors go to stderr. `--help` and `--version` still +print normal text and exit successfully. A literal `--json` after `--` is a +positional value, not an output flag. + ## Structured extraction ```sh diff --git a/crates/bsk-cli/src/cli/error.rs b/crates/bsk-cli/src/cli/error.rs index b764aaea..163e21dd 100644 --- a/crates/bsk-cli/src/cli/error.rs +++ b/crates/bsk-cli/src/cli/error.rs @@ -9,11 +9,15 @@ //! Exit codes are documented in [`super::render_error`] and //! design §3.1. +use std::ffi::OsString; use std::io::Write; +use std::path::Path; use std::process::ExitCode; use anyhow::Error; use bsk_protocol::{ErrorCode, RpcError}; +use clap::CommandFactory; +use clap::error::{ContextKind, ContextValue, ErrorKind}; use serde::Serialize; use thiserror::Error; @@ -107,6 +111,116 @@ pub enum Format { Json, } +/// Guidance for `--session` / `--tab-id` given before the subcommand. +const MISPLACED_COMMAND_OPTION_HINT: &str = "--session and --tab-id are command-specific \ + options; place them after a subcommand that supports them, e.g. `bsk click --session \ + --tab-id --selector '#submit'`; run `bsk --help` for supported options"; + +/// Render clap failures for `args` (the full argv clap parsed) using the +/// CLI's error contract, while preserving successful help/version output +/// and the existing usage-error exit code. +pub fn render_parse_error(err: clap::Error, args: &[OsString]) -> ExitCode { + if !err.use_stderr() { + let _ = err.print(); + return ExitCode::SUCCESS; + } + + // Parsing can fail before clap reaches --json. Only standalone flags + // before `--` select the error format; values stay literal. + let json = args + .iter() + .skip(1) + .take_while(|arg| *arg != "--") + .any(|arg| arg == "--json"); + let format = if json { Format::Json } else { Format::Human }; + let (err, hint) = match misplaced_command_option_error(&err, args) { + Some(rebuilt) => (rebuilt, Some(MISPLACED_COMMAND_OPTION_HINT)), + None => (err, None), + }; + let exit = exit_code_for(ErrorCode::InvalidParams); + match format { + Format::Json => { + let rendered = err.to_string(); + let error = CliError::Rpc { + code: ErrorCode::InvalidParams, + message: parse_error_summary(&err, &rendered), + data: Some(serde_json::json!({ + "reason": "cli_parse_error", + "details": rendered.trim_end(), + })), + source: None, + }; + let hint = hint.or_else(|| render_error::hint_for_code(ErrorCode::InvalidParams)); + println!("{}", json_error_string(&error, exit, hint)); + } + Format::Human => { + let _ = err.print(); + } + } + ExitCode::from(exit) +} + +/// clap's own did-you-mean can point `--session` at `--version` and print +/// `Usage: bsk --version `. Rebuild the error so its tip and usage +/// describe where the option actually belongs. +fn misplaced_command_option_error(err: &clap::Error, args: &[OsString]) -> Option { + let arg = match (err.kind(), err.get(ContextKind::InvalidArg)) { + (ErrorKind::UnknownArgument, Some(ContextValue::String(arg))) + if matches!(arg.as_str(), "--session" | "--tab-id") => + { + arg.clone() + } + _ => return None, + }; + let mut cmd = super::Cli::command(); + // Like clap, name the binary after argv[0], e.g. `bsk.exe` on Windows. + if let Some(name) = args + .first() + .and_then(|arg0| Path::new(arg0).file_name()) + .and_then(|name| name.to_str()) + { + cmd = cmd.bin_name(name); + } + // The usage clap printed names the suggested argument when it found one; + // otherwise it is the usage of the command being parsed, e.g. `bsk tab`. + let usage = match ( + err.get(ContextKind::SuggestedArg), + err.get(ContextKind::Usage), + ) { + (None, Some(ContextValue::StyledStr(usage))) => usage.clone(), + _ => cmd.render_usage(), + }; + let mut rebuilt = clap::Error::new(ErrorKind::UnknownArgument).with_cmd(&cmd); + rebuilt.insert(ContextKind::InvalidArg, ContextValue::String(arg)); + rebuilt.insert( + ContextKind::Suggested, + ContextValue::StyledStrs(vec![MISPLACED_COMMAND_OPTION_HINT.into()]), + ); + rebuilt.insert(ContextKind::Usage, ContextValue::StyledStr(usage)); + Some(rebuilt) +} + +/// One-line `message` for JSON callers. clap renders `error: ` +/// followed by blank-line separated tips and usage, which stay in +/// `data.details`. The summary can wrap, e.g. a missing required argument +/// or the possible values for an invalid one, so join its lines. +fn parse_error_summary(err: &clap::Error, rendered: &str) -> String { + // A bare subcommand group renders its help page instead of an error. + if err.kind() == ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand { + return "a subcommand is required but one was not provided".into(); + } + let rendered = rendered.strip_prefix("error: ").unwrap_or(rendered); + rendered + .split("\n\n") + .next() + .unwrap_or_default() + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .collect::>() + .join(" ") +} + /// `--json` error shape. Top-level fields (review I2 fix to round-1): /// `{ code, message, hint, exit_code, data }`. The previous shape /// nested everything under an `"error"` key — design §10 / §3.1 diff --git a/crates/bsk-cli/src/main.rs b/crates/bsk-cli/src/main.rs index cd19c71f..9d7f6b02 100644 --- a/crates/bsk-cli/src/main.rs +++ b/crates/bsk-cli/src/main.rs @@ -1,22 +1,16 @@ use std::process::ExitCode; -use clap::{Parser, error::ErrorKind}; +use clap::Parser; -use bsk::cli::error::{CliError, Format, render}; +use bsk::cli::error::{CliError, Format, render, render_parse_error}; use bsk::cli::status::Output; use bsk::{Cli, Command, cli}; fn main() -> ExitCode { - let cli = match Cli::try_parse() { + let args: Vec<_> = std::env::args_os().collect(); + let cli = match Cli::try_parse_from(&args) { Ok(cli) => cli, - Err(err) => { - let exit = match err.kind() { - ErrorKind::DisplayHelp | ErrorKind::DisplayVersion => 0, - _ => 1, - }; - let _ = err.print(); - return ExitCode::from(exit); - } + Err(err) => return render_parse_error(err, &args), }; // The daemon installs its own subscriber (with the rolling file diff --git a/crates/bsk-cli/tests/cli_parse.rs b/crates/bsk-cli/tests/cli_parse.rs index ccd7acff..ceb7deed 100644 --- a/crates/bsk-cli/tests/cli_parse.rs +++ b/crates/bsk-cli/tests/cli_parse.rs @@ -14,6 +14,51 @@ fn parse(args: &[&str]) -> Cli { Cli::try_parse_from(args).expect("clap parse should succeed") } +#[test] +fn global_output_flags_work_before_and_after_the_command() { + for args in [ + vec![ + "bsk", + "--json", + "--quiet", + "click", + "--session", + "s1", + "--tab-id", + "7", + ], + vec![ + "bsk", + "click", + "--session", + "s1", + "--tab-id", + "7", + "--json", + "--quiet", + ], + ] { + let cli = parse(&args); + assert!(cli.flags.json); + assert!(cli.flags.quiet); + let Command::Click(click) = cli.command else { + panic!("expected click"); + }; + assert_eq!(click.session, "s1"); + assert_eq!(click.tab_id, Some(7)); + } + for args in [ + vec!["bsk", "--json", "--quiet", "tab", "list", "--session", "s1"], + vec!["bsk", "tab", "--json", "--quiet", "list", "--session", "s1"], + vec!["bsk", "tab", "list", "--session", "s1", "--json", "--quiet"], + ] { + let cli = parse(&args); + assert!(cli.flags.json); + assert!(cli.flags.quiet); + assert!(matches!(cli.command, Command::Tab(_))); + } +} + #[test] fn parses_background_sessions_and_legacy_no_focus() { for unattended in [false, true] { diff --git a/crates/bsk-cli/tests/cli_usage_errors.rs b/crates/bsk-cli/tests/cli_usage_errors.rs new file mode 100644 index 00000000..27cdde70 --- /dev/null +++ b/crates/bsk-cli/tests/cli_usage_errors.rs @@ -0,0 +1,296 @@ +//! Process-level coverage of argument errors, output streams, and exit codes. + +use std::path::Path; +use std::process::{Command, Output}; + +use serde_json::Value; + +/// clap names the binary after argv[0]: `bsk.exe` on Windows. +fn usage(rest: &str) -> String { + let bin = Path::new(env!("CARGO_BIN_EXE_bsk")) + .file_name() + .unwrap() + .to_str() + .unwrap(); + format!("Usage: {bin} {rest}") +} + +fn bsk(args: &[&str]) -> Output { + let home = tempfile::tempdir().unwrap(); + Command::new(env!("CARGO_BIN_EXE_bsk")) + .args(args) + .env("BSK_HOME", home.path()) + .env("BSK_AUTO_UPDATE", "off") + .env("NO_COLOR", "1") + .output() + .expect("run bsk") +} + +fn json_usage_error(output: &Output) -> Value { + assert_eq!(output.status.code(), Some(1)); + assert!( + output.stderr.is_empty(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let error: Value = serde_json::from_slice(&output.stdout).unwrap_or_else(|err| { + panic!( + "expected JSON error: {err}; stdout: {}", + String::from_utf8_lossy(&output.stdout) + ) + }); + assert_eq!(error["code"], "invalid_params"); + assert_eq!(error["exit_code"], 1); + assert_eq!(error["data"]["reason"], "cli_parse_error"); + let message = error["message"].as_str().unwrap(); + assert!(!message.is_empty()); + assert!( + !message.contains('\n'), + "message should be one line: {message:?}" + ); + assert!(!message.starts_with("error:"), "{message:?}"); + assert!(!message.contains('\x1b'), "{message:?}"); + let details = error["data"]["details"].as_str().unwrap(); + assert!(!details.is_empty()); + assert!(!details.contains('\x1b'), "{details:?}"); + assert!(!error["hint"].as_str().unwrap().is_empty()); + assert!(error.get("error").is_none(), "keep the flat error contract"); + error +} + +#[test] +fn misplaced_session_and_tab_flags_report_json_and_position_guidance() { + for args in [ + vec!["--session", "s1", "click", "--selector", "#submit"], + vec!["--session=s1", "click", "--selector", "#submit"], + vec!["--tab-id", "7", "click", "--session", "s1"], + vec!["--tab-id=7", "click", "--session", "s1"], + vec!["tab", "--session", "s1", "list"], + ] { + // clap can stop before it reaches --json. Both placements must work. + for json_first in [true, false] { + let mut argv = args.clone(); + if json_first { + argv.insert(0, "--json"); + } else { + argv.push("--json"); + } + let error = json_usage_error(&bsk(&argv)); + let hint = error["hint"].as_str().unwrap(); + assert!(hint.contains("after"), "{hint}"); + assert!(hint.contains("bsk click --session"), "{hint}"); + let message = error["message"].as_str().unwrap(); + assert!(message.starts_with("unexpected argument '--"), "{message}"); + let details = error["data"]["details"].as_str().unwrap(); + assert!(details.contains(hint), "{details}"); + assert!(!details.contains("--version"), "{details}"); + } + } +} + +#[test] +fn all_argument_failures_use_the_json_contract() { + for args in [ + vec!["--json", "click"], // missing required session + vec!["click", "--json", "--session"], // missing option value + vec!["click", "--session", "s1", "--tab-id", "bad", "--json"], + vec!["click", "--session", "s1", "--button", "bad", "--json"], + vec!["unknown-command", "--json"], + vec!["status", "--unknown", "--json"], + vec!["--json"], // missing command + vec!["tab", "--json"], // missing nested command + vec!["--json", "tab"], // bare subcommand group renders help + vec!["--quiet", "--json", "click"], + vec!["--json", "--json", "status"], // conflicting flag + ] { + json_usage_error(&bsk(&args)); + } +} + +#[test] +fn json_message_is_the_clap_summary_and_details_keep_the_full_text() { + let missing = json_usage_error(&bsk(&["--json", "click"])); + assert_eq!( + missing["message"], + "the following required arguments were not provided: --session " + ); + assert!( + missing["data"]["details"] + .as_str() + .unwrap() + .contains(&usage("click --session ")) + ); + + let invalid = json_usage_error(&bsk(&[ + "click", + "--session", + "s1", + "--button", + "bad", + "--json", + ])); + assert_eq!( + invalid["message"], + "invalid value 'bad' for '--button