Telemetry: first-run notice and events cover every invocation

[sdairs]

Closes #320

Based on main, deliberately not stacked on #316: the two changesets are disjoint (#316 touches cloud models/commands and appends to cli_request_shape_test.rs; this PR touches main.rs, telemetry.rs, src/cli.rs, tests/telemetry_test.rs), so they merge cleanly in either order.

What

Any invocation of the binary — bare, --help, --version, mistyped commands — now shows the first-run notice on first run and produces a telemetry event on subsequent runs, under the unchanged consent state machine.

• Single-exit main (Telemetry: first-run notice and events must cover every invocation (bare, --help, --version, parse errors) #320 §1): exactly one process::exit, preceded by exactly one finalize(). Parse Err branches e.print() (clap keeps formatting), keep their existing help/version update-notice behavior, and fall through as (invocation, exit_code). Usage errors keep clap's exit 2 and spawn no update-cache refresh. The hidden telemetry send child stays the sole intended early exit.
• Definition-anchored lossy capture (§2): capture_lossy(root, argv, error) walks argv against the built clap Command tree — subcommand tokens descend and record the definition's name, --long tokens resolve to defined flags (value-taking flags consume the next token, so a value equal to a sibling subcommand name is never misrecorded), and the walk stops at the first unmatched token. Every recorded string is owned by the clap definitions; argv slices never enter the payload.
• Payload (§3): new outcome (closed vocabulary: ok | help | version | invalid_subcommand | unknown_argument | missing_subcommand | missing_required | invalid_value | other_parse_error) and suggestion (clap's definition-derived did-you-mean). The raw unmatched token is deliberately never recorded — a typo is indistinguishable from a pasted secret.
• Exit codes (§4): untouched; the numeric 2-collision with "cancelled" is disambiguated by outcome in analytics and is Redefine exit codes: move 'cancelled' off 2 to resolve clash with clap usage errors #319's to fix shell-side.
• Exe-path snapshot (§5): telemetry::init() at the top of main snapshots current_exe() into a OnceLock, fixing the Linux post-self-update dropped event; the telemetry send child interface is pinned as stable cross-version in cli.rs.

Verification

• Unit tests for capture_lossy: bare, root/nested help, -h/-V, typo + suggestion, unknown flag, flag-value-equals-subcommand-name, inline = values, -- handling, hostile-argv SECRET never on the wire.
• Subprocess e2e (wiremock + temp HOME): first --help prints the notice and writes the marker; bare → missing_subcommand/exit 2; hallucinated subcommand → invalid_subcommand with no raw token; typo → suggestion: "service".
• Gates green per commit: cargo test -p clickhousectl (429 unit + 18 e2e), clippy -D warnings, cargo fmt --check, cargo check --workspace --all-features, and the full suite with telemetry compiled out (--no-default-features).
• A fresh-context adversarial review probed hostile argv shapes (non-UTF-8 tokens, --flag=SECRET, closed pipes) and audited clap 4.6's suggestion contexts (SuggestedSubcommand/SuggestedArg are definition-derived; the input-embedding Suggested* kinds are not read). Its findings are fixed in the final commit: a closed stderr no longer turns exit 2 into a panic (writes are swallowed like clap's own Error::exit), and the pre-existing child-exit-code passthrough exits it spotted are acknowledged and tracked as Local child-process passthrough exits bypass the telemetry tail #321.

Rollout

The ingest worker must accept outcome/suggestion (backward compatible — deploy before the next CLI release). The docs page update ('help/version/failed invocations are recorded as such') is step 3; the notice text stays true and unchanged.

🤖 Generated with Claude Code

[sdairs]

Finding: short flags (other than -h/-V) are invisible to capture_lossy.

The lossy walker in telemetry.rs resolves --long flags via get_long() and hardcodes the two implicit shorts (-h/-V), but any other single-dash token falls through to the else { break } — the walk stops and nothing after it is recorded.

The CLI has real short flags today: -v (local server start/postgres … --version), -F (--foreground), -p (--port), -q (--query), -e (--env). So e.g. a failing clickhousectl local client -q "select 1"-style invocation would record only command: "local client" and lose everything after the short flag.

This is privacy-safe (fails toward recording less, never toward leaking a value), and it diverges from capture, which resolves -h/-V via normal definition lookup rather than special-casing. Not a blocker for this PR — but worth a follow-up to resolve shorts via get_short() the same way longs are resolved via get_long(), or at minimum a comment at the else { break } acknowledging that single-dash tokens stop the walk. Right now that limitation is implicit and only discoverable by reading the walker closely.

[sdairs]

The docs link the notice points users to has moved and needs updating.

DOCS_URL (telemetry.rs:42), the after_help text on the telemetry command (cli.rs:87), and the README telemetry section (line 888) all still point to:

https://clickhouse.com/docs/interfaces/cli#telemetry

That URL now redirects; the canonical location is:

https://clickhouse.com/docs/concepts/features/interfaces/cli

Since this PR makes the first-run notice fire on every first invocation (bare, --help, typos — not just successful commands), far more users will see and follow that link than before. Worth updating all three spots to the new URL in this PR rather than shipping a notice that sends fresh users through a redirect.

[sdairs]

Structural review (no runtime checks)

Overall, the core direction is good: a common telemetry tail is the right shape, refusing to record unmatched tokens is the right privacy choice, and eagerly snapshotting current_exe() cleanly addresses the Linux self-update case. I would still request changes before approval because the implementation does not yet satisfy its central “every invocation reaches one telemetry tail” claim.

Findings

1. [P1] Successful exec() handoffs bypass telemetry entirely.

   run() is not guaranteed to return to main.rs:

   • crates/clickhousectl/src/local/mod.rs:299 uses CommandExt::exec() for local client.
   • crates/clickhousectl/src/local/postgres.rs:626 uses it for the host-psql path.

   On success, the clickhousectl process image is replaced, so telemetry::finalize() is never reached. Those invocations get no first-run notice, marker, or event. These paths are also absent from follow-up Local child-process passthrough exits bypass the telemetry tail #321, which only lists the three nested process::exit paths. Either finalize before an explicit handoff with a distinct outcome, or spawn/wait and propagate the child status through a typed top-level result.

2. [P2] capture_lossy implements only a subset of clap’s grammar.

   The walker handles canonical long names plus hard-coded -h/-V, but not long aliases such as --fg and --config-file, real short options such as -v, -q, and -e, short clusters, or attached short values. It stops at the first such token, so valid flags—and sometimes later useful context—are omitted. Bugbot already noted aliases; short options make this a broader parser-drift problem. The matcher should cover all spellings and value arity exposed by Arg, ideally with coverage generated from the built command tree.

3. [P2] Suggestions are not definition-anchored by this code.

   suggestion_for_error() directly clones strings from clap’s error context. They are safe with the currently locked clap implementation, but the public “never your input” promise depends on clap internals rather than an invariant enforced here. Validate any suggestion against the relevant command/argument definitions and emit the canonical definition-owned spelling. Also, clap 4.6 orders SuggestedSubcommand candidates from least to most similar, so first() records the weakest candidate; use the last candidate or model suggestions as a list.

4. [P2] outcome: "ok" means “parsed”, not “succeeded”.

   capture() runs before dispatch and hard-codes ok, so handler failures, authentication errors, and cancellations produce outcome: "ok" alongside a non-zero exit code. That conflicts with the README description of this field as “how the invocation ended” and will be easy to misuse in analytics. Prefer parsed/dispatched, or separate parse outcome from execution outcome.

5. [P2] Cached update notices can still bypass finalization on closed stderr.

   main.rs now swallows clap and telemetry-notice write failures, but update::print_cached_update_notice() still uses eprintln!, which panics if stderr is closed. With a cached newer version, --help or --version can therefore panic before capture/finalize. The update notice should use the same non-panicking write pattern.

Architectural and rollout notes

• The “single-exit invariant” is currently a comment-level convention: lower layers still contain three acknowledged process::exit calls and two overlooked exec() handoffs. A typed RunDisposition/ExitStatus returned from handlers would make the boundary real and keep process termination in one place.
• The worker rollout must accept both old payloads where outcome/suggestion are absent and new payloads where suggestion is explicitly null. Delivery failures are intentionally silent, so a schema mismatch will silently drop new-version events.
• The external telemetry documentation should ship before or with the CLI release because disclosure now materially includes help, version, and failed invocations.
• The hidden cross-version telemetry send contract is protected only by a comment. A compatibility test or pre-parse child-mode guard would make self-update safer.
• README’s “Every invocation counts” statement is currently false because of the known exits and the two exec() paths above.

Recommendation: fix the exec() coverage before merge, and either fix the remaining P2 items or explicitly narrow the claims/docs and track them as concrete follow-ups.

[Copilot]

Pull request overview

Extends clickhousectl’s telemetry so every invocation (including --help, --version, bare invocation, and clap parse errors) participates in the existing consent flow and produces a structured event after consent, while keeping the “definition-derived names only” privacy guarantee.

Changes:

• Refactors main to a single common tail that always runs telemetry::finalize() and exits once, with parse errors handled via Error::print() + capture_lossy.
• Adds capture_lossy + outcome/suggestion telemetry fields to represent parse outcomes without ever recording unmatched raw tokens; snapshots current_exe() at startup for reliable child spawning post-self-update.
• Updates README telemetry documentation and adds/extends telemetry E2E tests for help/bare/invalid invocations.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
README.md	Documents new “every invocation counts” behavior and the new outcome/suggestion fields.
crates/clickhousectl/tests/telemetry_test.rs	Adds subprocess tests asserting notice + marker on first --help and telemetry events for bare/invalid invocations.
crates/clickhousectl/src/telemetry.rs	Implements lossy capture for parse errors, adds outcome/suggestion, and snapshots exe path at startup for send-child spawning.
crates/clickhousectl/src/main.rs	Restructures parsing/dispatch to always reach a single finalize+exit tail; collects argv once to support lossy capture.
crates/clickhousectl/src/cli.rs	Pins the hidden telemetry send subcommand as a stable cross-version interface.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 2f9e85c. Configure here.}

[cursor]

Suggestion lookup ignores flag aliases

Low Severity

find_defined_name only matches argument long names and subcommand names when anchoring clap’s did-you-mean text, but capture_lossy treats long-flag aliases (e.g. config-file, fg) as valid tokens. When clap’s suggestion uses an alias string, anchoring fails and suggestion is omitted even though a definition-owned canonical name could be recorded safely.

 

^{Reviewed by Cursor Bugbot for commit 2f9e85c. Configure here.}