Bring help, version, and bare invocations into the telemetry lifecycle

[sdairs]

Closes #309

What

--help/-h/subcommand help, --version, bare clickhousectl, and incomplete command groups previously exited inside the clap Err match before the telemetry module ran — no first-run notice, no telemetry.json marker, no event, ever. They now join the normal telemetry lifecycle under the same consent rules, and the update notice is printed consistently across --help and the bare invocation (which keeps clap's usage-error exit code 2).

How

• telemetry::capture_from_args — no ArgMatches exists on the clap error path, so the invocation is re-derived from raw argv against the built clap definitions. The names-only discipline holds structurally: every recorded string is cloned from clap metadata (Command::get_name / Arg::get_long), never from a user-supplied token. Unresolvable tokens, values following value-taking flags, and everything after -- are skipped.
• main.rs Err arm — DisplayHelp/DisplayVersion (exit 0) and DisplayHelpOnMissingArgumentOrSubcommand/MissingSubcommand (exit 2, clap 4 splits flag-less vs. flag-bearing incomplete groups into these two kinds) all fall through to the update notice + telemetry::finalize. Genuinely mistyped invocations (typos, unknown flags) keep the intentional no-event _ => e.exit() behavior.
• Payload schema unchanged: help/version surface as flag names (["help"]/["version"]), a bare invocation as command:"", incomplete groups as a group path no successful run can produce, all with the real exit code.

Invariants preserved

• Homebrew-style consent: the notice-showing run sends nothing; first event on the next consented run (pinned by test on the help path).
• DO_NOT_TRACK overrides everything; unwritable config dir fails open to silent; the hidden telemetry send child never produces its own event.

Tests

• 10 new unit tests for capture_from_args (path depth, value/positional non-leakage incl. --flag=value, value tokens matching subcommand names, -- terminator, unknown tokens, non-UTF-8 tokens). The test helper attempts the same failing parse before build() so partially-built clap state matches production exactly.
• 7 new integration tests in telemetry_test.rs covering first-run notice/marker on --help, event shapes for help/version/bare/incomplete, update-notice consistency on the bare invocation, typo remaining event- and marker-free, and DNT on the help path.

Note for deployment

The ingest worker (separate repo) should be confirmed to tolerate command: "" (bare invocation events) — the schema is otherwise unchanged.

🤖 Generated with Claude Code

[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 7257eb3. Configure here.}

[cursor]

Non-UTF8 breaks value skip

Low Severity

In capture_from_args, when a value-taking flag sets skip_next_value and the next argv token is non-UTF-8, the loop continues without clearing that flag. The following token is then treated as a flag value and skipped, so later flags (e.g. help) can be omitted from telemetry even though clap still handled the invocation.

 

^{Reviewed by Cursor Bugbot for commit 7257eb3. Configure here.}

[macroscopeapp]

🟡 Medium src/telemetry.rs:293

capture_from_args discards a token after a non-UTF-8 value. When a value-taking flag is followed by a non-UTF-8 OsString, the token.to_str() check at line 294 hits continue while skip_next_value stays true, so the next real token is wrongly consumed as the value. For example, cloud --api-key <non-UTF-8> --help records api-key but omits help, producing an inaccurate telemetry event. Clear skip_next_value before the UTF-8 check so the flag's value is skipped regardless of whether it is valid UTF-8.

Suggested change

	// 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;
	}
	// Non-UTF-8 tokens can only be values; skip them.
	let Some(token) = token.to_str() else {

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @crates/clickhousectl/src/telemetry.rs around lines 293-296:

`capture_from_args` discards a token after a non-UTF-8 value. When a value-taking flag is followed by a non-UTF-8 `OsString`, the `token.to_str()` check at line 294 hits `continue` while `skip_next_value` stays `true`, so the next real token is wrongly consumed as the value. For example, `cloud --api-key <non-UTF-8> --help` records `api-key` but omits `help`, producing an inaccurate telemetry event. Clear `skip_next_value` before the UTF-8 check so the flag's value is skipped regardless of whether it is valid UTF-8.

[sdairs]

Review: structural assessment

Reviewed the issue (#309) and the full diff. Overall this is a sound approach, not a hack — the scope is justified and the privacy invariants hold. A few items below.

The approach is correct

The fundamental constraint is that clap gives no ArgMatches on the error path (try_get_matches_from_mut returns Err), so telemetry::capture() — which walks ArgMatches ids — can't be reused. The alternatives are all worse: re-parsing without --help/--version is fragile and doesn't cover bare/incomplete invocations; clap's error context doesn't expose enough to reconstruct the flag set. A parallel capture_from_args that walks raw argv against clap's Command/Arg metadata is the right answer.

The main.rs restructuring is clean — returning an exit code from the match and running shared post-match code (update notice + telemetry + exit) correctly unifies the update-notice behavior across --help and bare invocation (the asymmetry the issue flagged). --version still force-refreshes the cache before the shared notice print, preserving ordering.

The names-only privacy invariant is maintained by construction in both implementations: every recorded string is cloned from clap definitions (Command::get_name / Arg::get_long), never from a user token. Test coverage is thorough (10 unit + 7 integration).

Should fix before merge

Non-UTF-8 + skip_next_value ordering bug (telemetry.rs:292-300). When a value-taking flag sets skip_next_value and the next token is non-UTF-8, token.to_str() hits continue without clearing the flag. The following token is then wrongly consumed as the value, so later flags (e.g. --help) can be omitted from the event. Fix is a one-line swap — clear skip_next_value before the UTF-8 check:

if skip_next_value {
    skip_next_value = false;
    continue;
}
// Non-UTF-8 tokens can only be values; skip them.
let Some(token) = token.to_str() else {
    continue;
};

This is exactly the kind of edge case a second parser implementation invites — clap's battle-tested parser handles this, the hand-rolled one doesn't.

Deployment item

The PR notes the ingest worker should be confirmed to tolerate command: "" (bare invocation events). Worth verifying before this ships.

Minor / optional

• The two parallel implementations (capture via ArgMatches, capture_from_args via raw argv) must stay in sync on the names-only contract. A brief coupling note in capture_from_args pointing back to capture would make the "keep in sync" obligation explicit for future maintainers.
• cmd.build() on a partially-built command (main.rs:78) works today and is well-documented, but is a mild fragility point dependent on clap's tolerance for double-building. No better option given the constraints.

[sdairs]

Structural review

I reviewed this change specifically for whether it's a sound foundation or a hacky fix, given how wide-ranging it looks. Verdict: the structure is sound. The "wide" feel is mostly tests — of the 434 added lines, roughly 300 are unit/integration tests. The production change is ~130 lines of new capture logic plus a main.rs restructure that made the error path simpler, not more special-cased. All 28 telemetry unit tests pass locally.

Why some new mechanism was unavoidable

The root cause is a clap design decision we can't route around: help, version, and incomplete-subcommand invocations are modeled as parse errors, so no ArgMatches ever exists on those paths and the existing telemetry::capture has nothing to walk. The alternatives to capture_from_args are all worse:

• Disabling clap's auto help/version and modeling them as ordinary flags would touch every command enum in cli.rs and forfeit clap's help propagation — that would have been the genuinely invasive change.
• Stripping --help tokens and re-parsing still fails for bare and incomplete-group invocations (the same error class), so error-path handling is needed regardless.
• Scraping clap's rendered error/usage text is fragile string parsing.
• Command::ignore_errors is documented as best-effort, unsuitable as a foundation.

Why the shadow parser is acceptable here

capture_from_args is a partial re-implementation of clap's argv walk, which would normally be a red flag. What makes it defensible is the asymmetry in its failure modes: every recorded string is cloned from clap metadata (Command::get_name / Arg::get_long), never from a user token, and anything unresolvable is dropped. When it diverges from real clap semantics, the worst outcome is an under-recorded or distorted path in an analytics event — never a leaked value, never a behavior change. A shadow parser feeding telemetry only needs to be approximately right, and the doc comments show this trade was made deliberately.

The main.rs change is a net improvement: three arms that each did their own exit() converged into "compute exit code, then shared tail" (update notice → finalize → exit). That convergence fixes the bare-invocation update-notice inconsistency as a natural consequence rather than another special case, and typos still hit _ => e.exit(), so the intentional no-event-on-mistype behavior survives (pinned by test). Everything downstream reuses the existing pipeline — same Invocation type, same decide state machine, unchanged payload schema.

Requests

1. Add a consistency test tying the two capture implementations together. capture (from ArgMatches) and capture_from_args (from argv) are parallel implementations of the same names-only discipline, and nothing currently catches drift between them. For a list of successfully parseable invocations, assert capture_from_args produces the same Invocation as capture. Cheap, and it's the only thing that will catch divergence.
2. Document the clap-settings assumptions the shadow parser depends on. It doesn't understand infer_long_args / infer_subcommands — neither is used today, but enabling one would make capture_from_args silently under-record with no signal. Similarly, skip_next_value consumes exactly one token, so a future num_args(2..) flag could let a trailing value that matches a subcommand name distort the path (benign, but path-quality erosion). A short comment in cli.rs or a guard test naming these assumptions would future-proof it. (The one value_delimiter = ',' in the CLI today is inline-only and safe.)
3. Close the ingest-worker question before merge. The PR body says command: "" tolerance "should be confirmed" — bare-invocation events start flowing immediately on release, so this should actually be verified, not just flagged.

Minor warts, fine to leave

• Exit code 2 now means both "cancelled" and "usage error" in analytics — distinguishable, since incomplete-group events carry a path no successful run produces, and the README was updated.
• help cloud records "help" while cloud --help records "cloud" + help flag — tested and documented; resolving the help topic would add complexity for little gain.
• The new arm uses let _ = e.print() where the older arms use .expect(...) — cosmetic inconsistency.

🤖 Generated with Claude Code