diff --git a/README.md b/README.md index 4e622c0..4098951 100644 --- a/README.md +++ b/README.md @@ -891,12 +891,14 @@ Each event contains exactly: - the command path (e.g. `local start`) - the **names** of the flags passed (e.g. `json`, `org-id`) — never flag values, never positional arguments -- the exit code (`gh`-style: 0 success, 1 error, 2 cancelled, 4 auth required) +- the exit code (`gh`-style: 0 success, 1 error, 2 cancelled/usage error, 4 auth required) - the CLI version, OS, and architecture - whether it ran in CI (`CI` env var) - whether it ran under a detected coding agent, and if so which one (e.g. `claude-code`) -There is no install ID, no device ID, and no fingerprinting of any kind. The payload is built from the clap command definitions rather than the raw command line, so leaking an argument value is structurally impossible — the code that builds the event has no access to values at all. +There is no install ID, no device ID, and no fingerprinting of any kind. Every string in the event is cloned from the clap command definitions — subcommand and flag names as clap defines them — never from what you actually typed, so leaking an argument value is structurally impossible. + +Help, version, bare, and incomplete invocations (`--help`, `--version`, `clickhousectl` with no subcommand, or a command group without one) count as usage like any other command and produce the same names-only events under the same consent rules. Genuinely mistyped invocations (unknown commands or flags) never produce an event. Nothing is ever sent before you have seen the notice or explicitly enabled telemetry with `clickhousectl telemetry enable`: the first run prints a one-time notice to stderr, records that it was shown in `~/.clickhouse/telemetry.json`, and sends nothing. Sending starts from the following run — or immediately if you opt in by running `telemetry enable`, which is explicit consent and skips the notice. The send happens in a short-lived detached process, so command latency is unaffected even when the endpoint is unreachable. diff --git a/crates/clickhousectl/src/main.rs b/crates/clickhousectl/src/main.rs index 2cd7235..f9a2595 100644 --- a/crates/clickhousectl/src/main.rs +++ b/crates/clickhousectl/src/main.rs @@ -41,25 +41,45 @@ async fn main() { let matches = match cmd.try_get_matches_from_mut(std::env::args_os()) { Ok(matches) => matches, Err(e) => { - match e.kind() { - // --version always hits the network to refresh the cache + timer, - // then prints the notice from the freshly-updated cache. + let exit_code = match e.kind() { + // --version always hits the network to refresh the cache + + // timer; the notice below then prints from the fresh cache. ErrorKind::DisplayVersion => { e.print().expect("failed to print output"); update::force_refresh_update_cache().await; - update::print_cached_update_notice(); - std::process::exit(0); + e.exit_code() // 0 } // --help shows the notice from cache (no blocking network call). ErrorKind::DisplayHelp => { e.print().expect("failed to print output"); - update::print_cached_update_notice(); - std::process::exit(0); + e.exit_code() // 0 } - // Parse errors exit here, before telemetry capture: a mistyped - // invocation never produces an event. + // Structurally valid but incomplete: bare `clickhousectl` + // (full help), or a command group given flags but no + // subcommand (usage error). Both print to stderr and keep + // clap's usage-error exit code, but participate in the + // update notice and telemetry below like --help does. + ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand + | ErrorKind::MissingSubcommand => { + let _ = e.print(); + e.exit_code() // 2 + } + // Genuine parse errors (typos, unknown flags) exit here, + // before telemetry capture: a mistyped invocation never + // produces an event. _ => e.exit(), + }; + update::print_cached_update_notice(); + // No `ArgMatches` exists on this path, so the invocation is + // re-derived from raw argv against the built clap definitions — + // same names-only discipline, same consent rules as any command. + #[cfg(feature = "telemetry")] + { + cmd.build(); + let args: Vec = std::env::args_os().skip(1).collect(); + telemetry::finalize(telemetry::capture_from_args(&cmd, &args), exit_code); } + std::process::exit(exit_code); } }; diff --git a/crates/clickhousectl/src/telemetry.rs b/crates/clickhousectl/src/telemetry.rs index e0db504..23ca0d3 100644 --- a/crates/clickhousectl/src/telemetry.rs +++ b/crates/clickhousectl/src/telemetry.rs @@ -12,9 +12,11 @@ //! everything: no notice, no file write, no send. //! //! The payload carries the command path and flag *names* only — never flag -//! values, never positional arguments. It is built from the clap definitions -//! ([`capture`] walks `ArgMatches` ids and `Arg` metadata, never touching -//! `get_one`/`get_raw`), so leaking a value is structurally impossible. +//! values, never positional arguments. Every recorded string is cloned from +//! the clap definitions ([`capture`] walks `ArgMatches` ids and `Arg` +//! metadata; [`capture_from_args`] resolves raw tokens against `Command` and +//! `Arg` metadata and drops whatever doesn't resolve), so a user-supplied +//! token can never appear in an event. //! //! Transport is a detached child process (`clickhousectl telemetry send`, //! hidden): the parent spawns it with all stdio nulled and never waits, so @@ -240,6 +242,107 @@ pub fn capture(root: &clap::Command, matches: &clap::ArgMatches) -> Invocation { } } +/// Derive the command path and passed-flag names from raw argv (without +/// argv[0]), for invocations that never produced `ArgMatches`: help, version, +/// bare, and incomplete-subcommand invocations surface as clap errors before +/// parsing completes (issue #309). +/// +/// Same names-only discipline as [`capture`], enforced differently: the walk +/// reads raw tokens, but every recorded string is cloned from the clap +/// definitions — `Command::get_name` for subcommands, `Arg::get_long` for +/// flags — never from a user-supplied token. Tokens that don't resolve +/// against the definitions (values, positionals, typos) are skipped, `--` +/// ends the walk, and the token after a value-taking flag is consumed as its +/// value — so a value that coincidentally matches a subcommand name usually +/// cannot extend the path (an unconsumable value token still can, which at +/// worst distorts the recorded *path*, never leaks the token itself). +/// +/// `root` must have had [`clap::Command::build`] called: clap's auto +/// `--help`/`--version` args only exist on built commands. +pub fn capture_from_args(root: &clap::Command, args: &[std::ffi::OsString]) -> Invocation { + /// Innermost-first lookup of a long flag (or long alias) on the command + /// stack; globals are propagated downward at build time, so either level + /// resolves. + fn find_long<'a>(stack: &[&'a clap::Command], name: &str) -> Option<&'a clap::Arg> { + stack.iter().rev().find_map(|cmd| { + cmd.get_arguments().find(|a| { + a.get_long() == Some(name) + || a.get_all_aliases().is_some_and(|al| al.contains(&name)) + }) + }) + } + + fn find_short<'a>(stack: &[&'a clap::Command], c: char) -> Option<&'a clap::Arg> { + stack.iter().rev().find_map(|cmd| { + cmd.get_arguments().find(|a| { + a.get_short() == Some(c) + || a.get_all_short_aliases().is_some_and(|al| al.contains(&c)) + }) + }) + } + + fn takes_values(arg: &clap::Arg) -> bool { + arg.get_num_args().is_some_and(|r| r.takes_values()) + } + + let mut path: Vec<&str> = Vec::new(); + let mut stack: Vec<&clap::Command> = vec![root]; + let mut flags = std::collections::BTreeSet::new(); + let mut skip_next_value = false; + for token in args { + // Non-UTF-8 tokens can only be values; skip them. + let Some(token) = token.to_str() else { + continue; + }; + if skip_next_value { + skip_next_value = false; + continue; + } + if token == "--" { + break; + } + if let Some(rest) = token.strip_prefix("--") { + // `--name` or `--name=value`; the inline value is never examined. + let (name, inline_value) = match rest.split_once('=') { + Some((name, _)) => (name, true), + None => (rest, false), + }; + if let Some(arg) = find_long(&stack, name) { + if let Some(long) = arg.get_long() { + flags.insert(long.to_string()); + } + skip_next_value = !inline_value && takes_values(arg); + } + } else if let Some(cluster) = token.strip_prefix('-').filter(|c| !c.is_empty()) { + // Short-flag cluster (`-h`, `-ab`). A value-taking short ends the + // cluster: the remainder (`-oVALUE`) or the next token is its value. + let mut chars = cluster.chars(); + while let Some(c) = chars.next() { + let Some(arg) = find_short(&stack, c) else { + break; + }; + flags.insert(arg.get_long().unwrap_or(arg.get_id().as_str()).to_string()); + if takes_values(arg) { + skip_next_value = chars.as_str().is_empty(); + break; + } + } + } else if let Some(sub_cmd) = stack + .last() + .expect("stack starts non-empty and only grows") + .find_subcommand(token) + { + path.push(sub_cmd.get_name()); + stack.push(sub_cmd); + } + // Anything else is a value or positional: skipped, never recorded. + } + Invocation { + command: path.join(" "), + flags: flags.into_iter().collect(), + } +} + // --------------------------------------------------------------------------- // Finalize (the per-invocation hook) // --------------------------------------------------------------------------- @@ -662,6 +765,133 @@ mod tests { assert!(inv.flags.is_empty()); } + // -- capture_from_args: raw argv, same names-only discipline -------------- + + /// Mirrors the error path in `main`: the parse is attempted (and fails) + /// on the same `Command` before `build()`, so partially-built state is + /// identical to production. + fn capture_raw(args: &[&str]) -> Invocation { + let mut cmd = crate::cli::Cli::command(); + let argv: Vec = std::iter::once("clickhousectl") + .chain(args.iter().copied()) + .map(Into::into) + .collect(); + let _ = cmd.try_get_matches_from_mut(&argv); + cmd.build(); + let args: Vec = args.iter().map(Into::into).collect(); + capture_from_args(&cmd, &args) + } + + #[test] + fn capture_from_args_root_help_and_version() { + for args in [&["--help"][..], &["-h"][..]] { + let inv = capture_raw(args); + assert_eq!(inv.command, ""); + assert_eq!(inv.flags, ["help"]); + } + for args in [&["--version"][..], &["-V"][..]] { + let inv = capture_raw(args); + assert_eq!(inv.command, ""); + assert_eq!(inv.flags, ["version"]); + } + } + + #[test] + fn capture_from_args_bare_invocation_is_empty() { + let inv = capture_raw(&[]); + assert_eq!(inv.command, ""); + assert!(inv.flags.is_empty()); + } + + #[test] + fn capture_from_args_records_full_subcommand_path() { + let inv = capture_raw(&["cloud", "service", "list", "--help"]); + assert_eq!(inv.command, "cloud service list"); + assert_eq!(inv.flags, ["help"]); + } + + #[test] + fn capture_from_args_incomplete_groups() { + let inv = capture_raw(&["local"]); + assert_eq!(inv.command, "local"); + assert!(inv.flags.is_empty()); + + let inv = capture_raw(&["cloud", "--json"]); + assert_eq!(inv.command, "cloud"); + assert_eq!(inv.flags, ["json"]); + } + + #[test] + fn capture_from_args_never_records_values_or_positionals() { + let inv = capture_raw(&[ + "cloud", + "service", + "get", + "SECRET-SERVICE-ID", + "--org-id", + "SECRET-ORG", + "--help", + ]); + assert_eq!(inv.command, "cloud service get"); + assert_eq!(inv.flags, ["help", "org-id"]); + let json = serde_json::to_string(&build_payload(&inv, 0, &env_of(&[]))).unwrap(); + assert!(!json.contains("SECRET"), "payload leaked a value: {json}"); + + // The inline-equals form is equally unexaminable. + let inv = capture_raw(&["cloud", "service", "get", "--org-id=SECRET-ORG", "--help"]); + assert_eq!(inv.flags, ["help", "org-id"]); + let json = serde_json::to_string(&build_payload(&inv, 0, &env_of(&[]))).unwrap(); + assert!(!json.contains("SECRET"), "payload leaked a value: {json}"); + } + + #[test] + fn capture_from_args_value_matching_subcommand_name_is_not_descended() { + // "service" is a value here (consumed by the value-taking --api-key), + // not a path segment — even though it names a `cloud` subcommand. + let inv = capture_raw(&["cloud", "--api-key", "service", "--help"]); + assert_eq!(inv.command, "cloud"); + assert_eq!(inv.flags, ["api-key", "help"]); + } + + #[test] + fn capture_from_args_skips_unknown_tokens() { + let inv = capture_raw(&["clout", "--frobnicate", "-Z"]); + assert_eq!(inv.command, ""); + assert!(inv.flags.is_empty()); + } + + #[test] + fn capture_from_args_stops_at_double_dash() { + let inv = capture_raw(&["local", "--", "--help"]); + assert_eq!(inv.command, "local"); + assert!(inv.flags.is_empty()); + } + + #[test] + fn capture_from_args_help_subcommand() { + // The auto help subcommand has no nested subcommands of its own, so + // the topic token is a positional and only "help" is recorded. + let inv = capture_raw(&["help", "cloud"]); + assert_eq!(inv.command, "help"); + assert!(inv.flags.is_empty()); + } + + #[cfg(unix)] + #[test] + fn capture_from_args_skips_non_utf8_tokens() { + use std::os::unix::ffi::OsStringExt; + let args = vec![ + std::ffi::OsString::from("local"), + std::ffi::OsString::from_vec(vec![0x66, 0x6f, 0x80, 0x6f]), + std::ffi::OsString::from("--help"), + ]; + let mut cmd = crate::cli::Cli::command(); + cmd.build(); + let inv = capture_from_args(&cmd, &args); + assert_eq!(inv.command, "local"); + assert_eq!(inv.flags, ["help"]); + } + #[test] fn state_path_is_telemetry_json_under_base_dir() { let path = state_path().unwrap(); diff --git a/crates/clickhousectl/tests/telemetry_test.rs b/crates/clickhousectl/tests/telemetry_test.rs index 854215b..8f29a34 100644 --- a/crates/clickhousectl/tests/telemetry_test.rs +++ b/crates/clickhousectl/tests/telemetry_test.rs @@ -472,6 +472,174 @@ async fn parent_never_waits_for_a_slow_endpoint() { ); } +// --- Issue #309: help, version, bare, and incomplete invocations ----------- + +#[tokio::test] +async fn first_run_help_writes_marker_prints_notice_sends_nothing() { + let sandbox = Sandbox::new().await; + + let output = sandbox.run(&["--help"]); + assert_eq!(output.status.code(), Some(0)); + assert!(stdout_of(&output).contains("Usage:")); + let stderr = stderr_of(&output); + assert!( + stderr.contains("anonymous usage data") && stderr.contains("telemetry disable"), + "a first-ever --help must print the notice, got stderr: {stderr}" + ); + assert_eq!( + std::fs::read_to_string(sandbox.state_path()).unwrap(), + r#"{"disabled":false}"# + ); + // The notice-showing run itself sends nothing (Homebrew-style consent). + sandbox.assert_no_requests().await; + + // Second --help: notice appears exactly once ever; now-consented run + // sends an event under the same rules as any other command. + let output = sandbox.run(&["--help"]); + assert_eq!(output.status.code(), Some(0)); + assert!(!stderr_of(&output).contains("anonymous usage data")); + let payloads = sandbox.wait_for_requests(1).await; + assert_eq!(payloads[0]["command"], ""); + assert_eq!(payloads[0]["flags"], serde_json::json!(["help"])); + assert_eq!(payloads[0]["exit_code"], 0); +} + +#[tokio::test] +async fn subcommand_help_sends_full_command_path() { + let sandbox = Sandbox::new().await; + sandbox.write_state(false); + + let output = sandbox.run(&["cloud", "service", "list", "--help"]); + assert_eq!(output.status.code(), Some(0)); + + let payloads = sandbox.wait_for_requests(1).await; + assert_eq!(payloads[0]["command"], "cloud service list"); + assert_eq!(payloads[0]["flags"], serde_json::json!(["help"])); + assert_eq!(payloads[0]["exit_code"], 0); +} + +/// Note: `--version` force-refreshes the update cache against the real GitHub +/// API (10s timeout) before finalizing, so this test can be slow offline. +#[tokio::test] +async fn version_sends_event() { + let sandbox = Sandbox::new().await; + sandbox.write_state(false); + + let output = sandbox.run(&["--version"]); + assert_eq!(output.status.code(), Some(0)); + assert!(stdout_of(&output).contains(env!("CARGO_PKG_VERSION"))); + + let payloads = sandbox.wait_for_requests(1).await; + assert_eq!(payloads[0]["command"], ""); + assert_eq!(payloads[0]["flags"], serde_json::json!(["version"])); + assert_eq!(payloads[0]["exit_code"], 0); +} + +#[tokio::test] +async fn bare_invocation_prints_update_notice_and_sends_event() { + let sandbox = Sandbox::new().await; + sandbox.write_state(false); + + // Seed the update cache (format: `\n`, see + // update.rs::save_update_check) so the notice has something to say. + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs(); + std::fs::write( + sandbox + .home + .path() + .join(".clickhouse") + .join("last_update_check"), + format!("{now}\n99.0.0"), + ) + .unwrap(); + + let output = sandbox.run(&[]); + // Usage-error exit code is preserved; help goes to stderr. + assert_eq!(output.status.code(), Some(2)); + let stderr = stderr_of(&output); + assert!( + stderr.contains("Usage:"), + "bare run must print help: {stderr}" + ); + // The update notice is now consistent with --help. + assert!( + stderr.contains("There is a new version of clickhousectl"), + "bare run must print the update notice: {stderr}" + ); + + let payloads = sandbox.wait_for_requests(1).await; + assert_eq!(payloads[0]["command"], ""); + assert!(payloads[0]["flags"].as_array().unwrap().is_empty()); + assert_eq!(payloads[0]["exit_code"], 2); +} + +#[tokio::test] +async fn incomplete_subcommand_sends_group_path() { + let sandbox = Sandbox::new().await; + sandbox.write_state(false); + + // Bare group: clap prints full help on stderr. + let output = sandbox.run(&["local"]); + assert_eq!(output.status.code(), Some(2)); + let payloads = sandbox.wait_for_requests(1).await; + assert_eq!(payloads[0]["command"], "local"); + assert!(payloads[0]["flags"].as_array().unwrap().is_empty()); + assert_eq!(payloads[0]["exit_code"], 2); + + // Group with a flag but no subcommand: a different clap error kind + // (MissingSubcommand), same telemetry treatment. + let output = sandbox.run(&["cloud", "--json"]); + assert_eq!(output.status.code(), Some(2)); + let payloads = sandbox.wait_for_requests(2).await; + assert_eq!(payloads[1]["command"], "cloud"); + assert_eq!(payloads[1]["flags"], serde_json::json!(["json"])); + assert_eq!(payloads[1]["exit_code"], 2); +} + +/// Genuinely mistyped invocations stay event-free — and don't even trigger +/// the first-run notice or marker. Only well-formed help/version/bare/ +/// incomplete invocations joined the telemetry lifecycle. +#[tokio::test] +async fn mistyped_invocation_stays_event_free_and_markerless() { + let sandbox = Sandbox::new().await; + + let output = sandbox.run(&["clout"]); + assert_eq!(output.status.code(), Some(2)); + assert!(!stderr_of(&output).contains("anonymous usage data")); + + let output = sandbox.run(&["local", "list", "--frobnicate"]); + assert_eq!(output.status.code(), Some(2)); + assert!(!stderr_of(&output).contains("anonymous usage data")); + + assert!( + !sandbox.state_path().exists(), + "a mistyped invocation must not write the marker file" + ); + sandbox.assert_no_requests().await; +} + +#[tokio::test] +async fn do_not_track_help_is_fully_silent() { + let sandbox = Sandbox::new().await; + + let output = sandbox + .command(&["--help"]) + .env("DO_NOT_TRACK", "1") + .output() + .unwrap(); + assert_eq!(output.status.code(), Some(0)); + assert!(stdout_of(&output).contains("Usage:")); + assert!(!stderr_of(&output).contains("anonymous usage data")); + assert!( + !sandbox.state_path().exists(), + "DO_NOT_TRACK must not write the marker file on the help path" + ); + sandbox.assert_no_requests().await; +} + /// Check the marker file used by `Sandbox::state_path` matches what the /// binary actually writes — guards against the test suite silently diverging /// from the real path.