Repository navigation
fix(tracking): keep the user's arguments out of the telemetry command label - #4123
Merged
Merged
Conversation
📊 Automated PR Analysis
SummaryFixes a telemetry leak where the Review Checklist
Linked issues: #1298 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
Merged
aeppling
approved these changes
Sep 23, 2026
KuSh
force-pushed
the
fix/1298-telemetry-command-label
branch
from
September 23, 2026 19:57
d5d3b4b to
5fcd048
Compare
… label `low_savings_commands` labelled each command with the first three words of `rtk_cmd`. That field is RTK's own, but most of what it holds is the user's command line, so the third word is routinely an operand: `rtk ls /usr/bin`, `rtk curl https://…`, `rtk grep <pattern>`, and the TOML filter path stores the user's whole invocation after its `rtk:toml` prefix. Over a real history database, 201 of 2606 labels carried a path, a URL, a flag value or a search term. `top_passthrough` had the same leak and was fixed by grouping on the tool alone. That is too coarse here, because every label begins with `rtk` and the figure is only worth sending per filter -- `rtk git log` and `rtk git status` have to stay apart. So the label keeps the words RTK chose: the prefix, the tool as a basename, and a third word only for a tool that routes by a subcommand of its own, and only when it is shaped like one. Over the same database that is 0 labels carrying an argument, with the per-subcommand granularity intact. The router list is a superset of the tools that declare subcommands in `discover::rules`, cross-checked by a test rather than read at runtime, since nothing in `core` depends on `discover`. A tool missing from it keeps two words, which loses granularity and never leaks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KuSh
force-pushed
the
fix/1298-telemetry-command-label
branch
from
September 23, 2026 20:01
5fcd048 to
e58ae41
Compare
This was referenced Sep 26, 2026
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.
From the release review on #3979, which flagged
low_savings_commandsunder #1298. Independent of the six regression PRs — it shares no file with any of them and branches offdevelopdirectly.The leak
The telemetry label was the first three words of
rtk_cmd. That field is RTK's own, but most of what it holds is the user's command line, so the third word is routinely an operand:The TOML-filter path is worse: it stores the user's entire invocation after an
rtk:tomlprefix, so the label could carry ajqprogram and the file path it ran against.Measured over a real history database — 329,583 rows, 26,934 distinct
rtk_cmd— 201 of 2606 labels carried a path, a URL, a flag value or a search term.top_passthroughhad exactly this leak and was fixed in15f666ddby grouping on the tool alone.low_savings_commandswas left behind.Why "just take fewer words" doesn't work
Grouping on the first word collapses everything, since every label begins with
rtk. Taking two words loses the granularity that makes the figure worth sending —rtk git logandrtk git statusare different filters with different savings.Taking three words when the first is
rtkand two otherwise only moves 201 → 195: it fixes thertk:tomlfamily but not the main leak, because for a single-level command the third word is the operand.The distinction is not word count. It is whether the word is RTK's routing or the user's data.
What it does now
Keep the prefix, the tool as a basename, and a third word only when the tool routes by a subcommand of its own and that word is shaped like a subcommand (
^[a-z][A-Za-z0-9:_-]*$).take(3)(before)rtk? 3 : 2The basename step clears the last three (
rtk ./gradlew,rtk ./mvnw,rtk:toml /tmp/rtk-pr) and merges./gradlewwithgradlew, so the labels improve rather than degrade. Granularity is intact:rtk git log,rtk git status,rtk git show,rtk gh prall stay distinct.The router list
A superset of the tools that declare subcommands in
discover::rules, which is the authority for the ones it covers —command_label_matches_the_discover_rulesfails if a tool gains subcommands there and is not added here. The rest are listed because that table does not describe them: it carries no subcommands forgit, the most routed tool there is.Cross-checked by a test rather than read at runtime, because nothing in
coredepends ondiscover(the dependency runs the other way, 8 references) and this is not worth inverting it for.That test paid for itself three times on its first runs, each a real defect in this change:
sbt testOnly— the subcommand shape rejected camelCasephpstan— declares subcommands, was missing from the listbun pm ls— a two-word subcommand, which a three-word label cannot carry; it keepspm, coarser than the table and never an argumentA tool missing from the list keeps two words. That costs granularity and never leaks — the direction this has to fail in.
Not changed
The review also filed "telemetry query now admits negative-savings rows" under the same PR. That is deliberate and documented in the function's own docstring — "A net-regressing command (negative rate) is listed: it is the filter most in need of improvement" — added by the same commit that changed the predicate. It is also reachable, which the review could not establish:
rtk git worktreesits at −7.7 % over 2103 calls in a real database. Left as-is.Verification
6 unit tests: the real leaking shapes, the routed subcommands that must survive, an operand in the subcommand slot being dropped anyway (
HEAD~3..HEAD,--git-dir=…, a quoted path, a URL), degenerate input, thediscover::rulescross-check, and a property test asserting no label contains/,=,",',@,~or...cargo fmt --all,cargo clippy --all-targetsandcargo test --allare green on currentdevelop.🤖 Generated with Claude Code