Record positional argument presence in anonymous telemetry - #634
Open
sdairs wants to merge 1 commit into
Open
Conversation
sdairs
force-pushed
the
feat/480-telemetry-positionals
branch
from
August 28, 2026 14:46
92c9f07 to
ce0eb74
Compare
Telemetry recorded command paths and flag names but dropped all positional presence, so `local server stop` and `local server stop dev` produced identical events, and a bare `local use` parse failure was indistinguishable from a handler failure after a version was supplied (#480). The payload gains a `positionals` field carrying the definition-owned ids of the positional slots the user filled — presence only, never a value. Every entry is cloned from an `Arg` definition compiled into the binary, so the field's vocabulary is a closed set that cannot carry argv, exactly like the existing `suggestion` anchoring. Three exclusions keep it honest: - only `ValueSource::CommandLine` slots count, so clap defaults, environment-fed values, and handler-generated names stay absent — which is what makes "the user named it" and "we picked one" distinguishable, without a separate source field (the compatibility `--name` form already lands in `flags`) - passthrough slots are skipped via a structural marker test (`last`, `trailing_var_arg`, `allow_hyphen_values`), so argv forwarded to clickhouse-server/clickhouse-client/psql — including everything after `--` — is never classified as this CLI's shape - on the lossy path the walk already stops at `--` and never records the unmatched token; it now records the slot the token reached Tests: unit coverage for bare vs positional vs `--name` stop, supplied vs missing-required `local use`/`local remove`, defaulted positionals, value terminators, passthrough exclusion on both capture paths, a hostile secret-shaped positional fixture matrix, and a structural guard that every recorded id in the real command tree is a source identifier and every passthrough slot is recognized. End-to-end subprocess tests pin the same distinctions and the no-leak property on the wire. The first-run notice, `telemetry --help`, and the README telemetry section now state the positional privacy boundary and its exclusions. Fixes #480 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
sdairs
force-pushed
the
feat/480-telemetry-positionals
branch
from
August 28, 2026 19:41
ce0eb74 to
f33a195
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Anonymous telemetry recorded command paths and flag names but dropped all positional presence, so materially different invocations produced identical events:
local server stopvslocal server stop dev, and a barelocal useparse failure vs a handler failure after a version was supplied. That is what blocked the August 2026 telemetry investigation (#480).The payload gains a
positionalsfield carrying the definition-owned ids of the positional slots the user filled — presence only, never a value:{"command":"local server stop","flags":[],"positionals":["name"],"outcome":"ok", ...} {"command":"local server stop","flags":["name"],"positionals":[],"outcome":"ok", ...} {"command":"local use","flags":[],"positionals":[],"outcome":"missing_required", ...} {"command":"local remove","flags":[],"positionals":["version"],"outcome":"error", ...}Why it stays anonymous
Every entry is cloned from an
Argdefinition compiled into the binary — the same "definition-owned, never the user's input" anchoring the existingsuggestionfield uses — so the field's vocabulary is a closed set that structurally cannot carry argv. Three exclusions keep it honest:ValueSource::CommandLineslots count, so clap defaults, environment-fed values, and handler-generated names stay absent. That is exactly what makes "the user named it" and "we picked one" distinguishable, so no separatename_sourcefield was needed — the compatibility--nameform already lands inflags, so the two naming styles are already distinct.last,trailing_var_arg,allow_hyphen_values) rather than a hand-maintained id list, so argv forwarded toclickhouse-server/clickhouse-client/psql— including everything after--— is never classified as this CLI's shape, and new passthrough slots inherit the exclusion.--and never records the unmatched token; it now records only the slot the token reached.Tests
src/telemetry.rs): bare vs positional vs--namestop; supplied vs missing-requiredlocal use/local remove; defaulted positionals excluded; value terminators record nothing; passthrough exclusion on both capture paths; a hostile secret-shaped positional fixture matrix (AWS key,sk-live-…, Postgres URI with password, S3 path, SSH key path, GitHub token) asserted absent from serialized payloads; and a structural guard walking the real command tree that every recorded id is a source-level identifier and every passthrough slot is recognized.tests/telemetry_test.rs): subprocess + wiremock tests pinning the bare/named/flag-named stop distinction, missing-required vs supplied version, and no hostile value on the wire; existing payload-shape assertions extended withpositionals.cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test -p clickhousectl(all green), pluscargo check -p clickhousectl --no-default-features --all-targetsfor the telemetry-compiled-out configuration.Docs
The first-run notice, the
telemetrysubcommand's agent help, and the README telemetry section now state the positional privacy boundary and its exclusions (notice/help wording changed from "never values or arguments" to "flag and argument names (never their values)").Deploy note
The ingest worker's field allowlist needs a matching deploy before the next CLI release, or
positionalswill be dropped server-side (precedent: #320 / #322, which addedoutcome/suggestion). Dashboards built on pre-positionalsevents keep working — the field is additive and absent from older events.Part of a stacked PR chain: based on
fix/608-local-json-error-parity, notmain.Fixes #480
🤖 Generated with Claude Code