[maxmilian]

Summary

Fixes #1656. rtk telemetry status shows device hash: (no salt file) for two very different states:

1. Expected — consent never granted, so the salt is never attempted (by design).
2. Real failure — consent granted but the write was blocked (fs permission, disk full, …).

The single label reads as an error in case 1 and obscures actual failures in case 2.

Root cause

Verified against develop HEAD (per rg generate_device_hash and @truffle-dev's analysis):

• get_or_create_salt() already calls create_dir_all(parent) at src/core/telemetry.rs:175-177 — no fs bug.
• generate_device_hash() has three production call sites: send_ping() at telemetry.rs:71 (the only one that creates the salt; gated by consent_given == Some(true) at telemetry.rs:38-41), and run_status() / run_forget() (both only call it after salt_path.exists()).
• A fresh brew install rtk + rtk gain never triggers consent → no salt → (no salt file) is expected, not a failure.

The gating chain is documented inline on the new helper so future refactors don't have to re-derive it.

Fix

src/core/telemetry_cmd.rs:

• salt_missing_label(consent_given) -> &'static str returns:
  • (no salt file) when consent_given == Some(true) — real failure, original label preserved.
  • (telemetry not enabled; run \rtk telemetry enable` to opt in)otherwise — actionable.Some(false)(explicit opt-out) andNone` (never prompted) collapse because the remediation is identical.
• device_hash_line(consent_given, hash) -> String formats the full device hash: ... line. run_status() builds the Option<String> hash from salt_path.exists() and delegates.
• ASCII-only punctuation (semicolon, not em-dash) to match surrounding println! and stay grep / pipe friendly.

Tests

• cargo fmt --all --check && cargo clippy --all-targets -- -D warnings && cargo test — 1949 passed (3 new in telemetry_cmd::tests)
• salt_missing_label_distinguishes_consent_not_given_from_real_failure — all 3 consent states
• device_hash_line_renders_truncated_hash_when_salt_exists — consent-agnostic salt-present path
• device_hash_line_routes_to_salt_missing_label_when_no_hash — locks the routing so inlining or removing the helper without copying its literals fails CI
• Manual: rtk telemetry status output for salt_path.exists() users stays byte-identical (only the else branch changed)

Scope

Single-file change, ~85 LOC (test included). Deliberately does not:

• Touch get_or_create_salt() / send_ping() / consent prompt flow — no fs bug exists.
• Pre-create salt at brew postinstall — orthogonal design decision for maintainers.
• Trigger prompt_telemetry_consent() from rtk gain / rtk telemetry status — UX surface change, separate discussion.

Follow-up

Filed separately as #2106: cargo test writes a real .device_salt to the user's data dir because the unit tests in src/core/telemetry.rs (test_salt_is_persisted, test_device_hash_is_stable, test_device_hash_is_valid_hex) call the production functions with no path override. Hides the symptom of this issue on dev machines after the test suite runs. Not bundled here per CONTRIBUTING scope rules.

[CLAassistant]

All committers have signed the CLA.

[truffle-dev]

Read the diff against develop HEAD (878af7d) and traced the gating chain end-to-end. LGTM.

Gating proof verified. generate_device_hash() has the three production callers you named, and only one of them writes the salt — send_ping() at telemetry.rs:71, gated by match cfg.telemetry.consent_given { Some(true) => {}, Some(false) | None => return } at telemetry.rs:38-41. run_status() and run_forget() (this file at 21 and 109) both guard the call with salt_path.exists() first, so neither can create the salt on a no-consent install. get_or_create_salt() at telemetry.rs:162-186 does create_dir_all(parent) before writing — the parent isn't the failure surface.

So the proposed mapping is right: Some(true) no-salt is the only state that should keep (no salt file), and collapsing Some(false) | None into the same opt-in remediation matches the actual user-visible action.

Tests lock the routing. All three are pure-function tests over salt_missing_label / device_hash_line, no fs, no Config::load. The third one in particular (device_hash_line_routes_to_salt_missing_label_when_no_hash) is the one that catches a future cleanup re-inlining the literals without preserving them. Good shape.

One observation, not a blocker. device_hash_line's Some(h) if h.len() >= 64 guard is wider than the original. The pre-PR code did &hash[..8] and &hash[56..] unconditionally — a malformed hash would panic. The new code silently routes a len < 64 hash to salt_missing_label. SHA-256 is 64 hex chars by construction and get_or_create_salt() re-verifies cached salt with trimmed.len() == 64 && trimmed.chars().all(...) at telemetry.rs:169, so the guard never fires in practice. Worth keeping as defense-in-depth; just noting that the routing for a sub-64 hash would read as "telemetry not enabled" under Some(false) | None consent, which is the wrong message for that imaginary failure mode. If a maintainer prefers preserving the panic-on-malformed behavior, the gate could be Some(h) if h.len() == 64.

Follow-up #2106 is a separate fix in scope and is genuinely orthogonal — the test-pollution writes a real .device_salt regardless of which label the status command renders for it.

[maxmilian]

Good catch — adopted in 56f6afa. Tightened the guard to == 64 and added device_hash_line_treats_non_64_char_hash_as_missing to lock it: a non-64-length hash now routes to the salt-missing label instead of slicing an over-long tail. Left it as exact-match rather than defense-in-depth since the digest is 64 hex chars by construction.

[truffle-dev]

Confirmed against 56f6afa. The == 64 exact-match gate at device_hash_line is the right call given the digest is 64 hex chars by construction, and device_hash_line_treats_non_64_char_hash_as_missing (both the 16-char short and the 128-char long branch) pins the routing so a future cleanup that loosens the guard back to >= 64 will fail the test. LGTM.

[maxmilian]

Friendly check-in — this one's been quiet for a few weeks and looks ready from my side:

• Reviewed: @truffle-dev traced the gating chain end-to-end and confirmed LGTM against 56f6afa (the == 64 exact-match guard + the device_hash_line_treats_non_64_char_hash_as_missing test pinning the routing).
• CI green, mergeable against develop.

No action needed from reviewers if it's just in the queue — just flagging it as merge-ready. Happy to rebase onto the latest develop if that helps it land. 🙏

[maxmilian]

Gentle bump for @aeppling 🙏 — this one's been quiet for a few weeks and looks ready:

• Reviewed & LGTM'd twice by @truffle-dev, who traced the gating chain end-to-end and confirmed the == 64 exact-match guard against 56f6afa.
• Behavior is pinned by device_hash_line_treats_non_64_char_hash_as_missing, and the CLA check is green.

Anything I can do to help move it along? Happy to rebase on develop if it's drifted. Thanks!

[pszymkowiak]

Tested all three consent states live with the compiled binary — None, Some(false), and Some(true) (no salt) each render exactly as described. cargo fmt, cargo clippy --all-targets, cargo test --all all clean (1950/1950). Appreciate the precise scoping and the separate follow-up issue (#2106) instead of scope creep.

[maxmilian]

Rebased onto latest develop to clear the merge conflict — it came from the env opt-out refactor (single source of truth for env opt-out check) landing in the same file. Resolved as a union: kept that refactor's TELEMETRY_DISABLED_ENV constants + test_telemetry_disabled_by_env_honors_opt_out, plus this PR's salt_missing_label / device_hash_line helpers and their tests. No logic changes on either side.

cargo test telemetry green (20 passed, including both the env opt-out test and this PR's four new ones). Back to MERGEABLE / approved.

[maxmilian]

@aeppling — this one looks ready to land whenever you have a moment:

• Approved by @pszymkowiak on Jul 9 (after @truffle-dev traced the gating chain end-to-end and LGTM'd twice).
• Rebased onto latest develop on Jul 21 to clear the conflict from the env opt-out refactor — resolved as a union, both sides kept.
• cargo test telemetry --bins: 20 passed (my 4 + the develop env test).
• Currently MERGEABLE / clean, CI green, CLA signed.

No rush on my side — just flagging that there's nothing left blocking it technically. Happy to rebase again if develop moves.

[maxmilian]

Friendly ping — this one has an approval and is a single-file change, so I think it just needs a merge.

Recapping for whoever picks it up: the telemetry path currently can't tell "the user declined consent" apart from "the consent setting was never written", so a declined install gets treated as unconfigured on every subsequent run. The fix distinguishes the two states; one file, no behavior change for users who already consented.

Also — separate note, and the more useful half of this comment: I'm going to reduce my own open-PR count here. I opened seven PRs across 26–28 May, which in hindsight was too many at once from someone with no track record in this repo. I'm closing the four that have had no review activity (#2143, #2141, #2140, #2122) so the queue reflects what actually matters, and keeping this one plus #2142 (which has a real user report behind it — #1830 documents dotnet test running 1208× over 30 days at ~0% compression).

No urgency on either. Just didn't want to leave a wall of stale PRs sitting on your review list.

[KuSh]

Requesting changes. The split is the right idea and the tests are well-targeted, but it keys off consent_given alone — and consent is only one of five gates on salt creation. Three of the others produce "consent granted + no salt" legitimately, and in each of those rtk telemetry status now reports a filesystem failure that isn't happening. That's the same false-alarm class #1656 is about, moved onto a different set of users.

The gates, in the order maybe_ping() applies them (src/core/telemetry.rs:22-67):

1. TELEMETRY_URL.is_none() — no endpoint compiled into this build (:23-26)
2. telemetry_disabled_by_env() — RTK_TELEMETRY_DISABLED=1 (:29-30)
3. consent_given != Some(true) (:38-41) — the only one this PR models
4. !cfg.telemetry.enabled (:44-46)
5. the 23 h interval check (:50-58), with the salt itself written on a detached thread (:65-67)

run_status() already holds everything needed for 1, 2 and 4: env_override is computed at line 88, config.telemetry.enabled at 82, and send_erasure_request (line 176) already reads option_env!("RTK_TELEMETRY_URL") directly, so no new accessor in telemetry.rs is required.

Suggested fix

Give the label the rest of the state, and order the checks the way maybe_ping() orders its gates so the message names the one that actually fired:

fn salt_missing_label(
    endpoint_configured: bool,
    env_override: bool,
    enabled: bool,
    consent_given: Option<bool>,
) -> &'static str {
    // Mirrors the gate order in `telemetry::maybe_ping`.
    if !endpoint_configured {
        return "(not applicable; this build has no telemetry endpoint)";
    }
    if env_override {
        return "(blocked by RTK_TELEMETRY_DISABLED)";
    }
    match consent_given {
        Some(false) | None => "(telemetry not enabled; run `rtk telemetry enable` to opt in)",
        Some(true) if !enabled => "(telemetry disabled in config.toml)",
        Some(true) => "(no salt file yet; written on the first ping)",
    }
}

and at the call site:

    println!(
        "{}",
        device_hash_line(
            option_env!("RTK_TELEMETRY_URL").is_some(),
            env_override,
            config.telemetry.enabled,
            config.telemetry.consent_given,
            hash.as_deref(),
        )
    );

Five positional params is a lot — passing &config plus env_override is a fine alternative if you'd rather keep the signature narrow; the existing tests adapt either way.

The last arm is deliberately no longer an accusation: (no salt file yet; written on the first ping) is true both when the ping simply hasn't landed and when it genuinely failed, which is as far as the available state lets us go.

Everything else checks out: the hash.as_deref() plumbing is correct, the 01234567...89abcdef expectation matches &h[..8] / &h[56..] on the sample, the salt-exists path is byte-identical to pre-PR output as the description claims, and the branch still applies cleanly on develop after the July rebase.

[maxmilian]

Thanks @KuSh — the gap is real and I'm adopting your fix. One correction on the framing first, then what changed.

This doesn't move the false alarm onto a different set of users; it shrinks the set and then stops short. Before this PR, every no-salt install printed (no salt file) — including all four states you list. The PR routes Some(false) | None to the opt-in message and leaves the rest on the old string, so the users you're describing see exactly what they see on develop today. That doesn't make them less worth fixing: your actual point — that consent_given alone can't carry this label — stands, and in those three states (no salt file) still reads as a filesystem failure that isn't happening.

Verified your gate list against develop @ 9a695d1 (src/core/telemetry.rs; line numbers sit a couple lower than in your note):

• TELEMETRY_URL.is_none() — :24-26
• telemetry_disabled_by_env() — :29-31
• consent_given != Some(true) — :40-43
• !cfg.telemetry.enabled — :46-48
• the 23 h interval, then the salt written on the detached thread — :51-61 and :66

And the state really is all in hand at the call site (src/core/telemetry_cmd.rs): env_override at :50, config.telemetry.enabled at :44, and send_erasure_request already reads option_env!("RTK_TELEMETRY_URL") at :176.

Adopted salt_missing_label as you wrote it, ordered to mirror maybe_ping's gates, with the option_env! read at the call site so this stays a single-file change. I kept the four positional params rather than taking &config: the existing tests are pure-function calls that never construct a Config, and I'd rather not make them start.

Two notes on the result, neither of which changes the shape:

• run_status already prints enabled: no and env override: RTK_TELEMETRY_DISABLED=1 (blocked) as their own rows, so two of the new labels restate a line printed just above. That's fine — the device-hash row is the one that reads as an error, and it should stop doing that on its own terms.
• The interval gate needs no arm: (no salt file yet; written on the first ping) is already true for the case where the marker was touched but the detached thread never got the salt written. That's spelled out in the comment on the arm so nobody adds a fifth branch for it later.

The four existing tests moved to the new signatures, the routing test gained one case per new arm, and a new salt_missing_label_names_the_gate_that_actually_fired pins the ordering. I checked that last one actually bites rather than trusting it: changing the env gate to if env_override && consent_given == Some(true) — the plausible way to get the order wrong — turns it red with

assertion `left == right` failed
  left: "(telemetry not enabled; run `rtk telemetry enable` to opt in)"
 right: "(blocked by RTK_TELEMETRY_DISABLED)"
test result: FAILED. 20 passed; 1 failed

and green again once reverted.

Validation on the new head:

• cargo fmt --check — exit 0
• cargo clippy --all-targets -- -D warnings — exit 0, Finished dev profile ... in 8.57s, no warnings
• cargo test telemetry --bins — 21 passed; 0 failed (was 20; the five telemetry_cmd tests include the new ordering test)
• cargo test --all — 2443 passed; 0 failed; 8 ignored in the main binary, plus 6 / 11 / 11 / 14 / 5 / 11 passed across the integration targets, 0 failed anywhere

[KuSh]

Approving on bb85017. The gate-order rewrite in f40f0a1 addressed every blocking point — salt_missing_label now names the gate that actually fired, and the ordering is pinned by a test. Nice work.

Two housekeeping things I did directly on the branch, since you'd enabled maintainer edits:

Rebased onto current develop (4f3c9fe). The branch was ~300 commits behind and wouldn't have been mergeable. One conflict, in src/core/utils.rs, where develop added tests at the same spot as mine — resolved by keeping both. Your two commits are unchanged apart from the new base and keep their authorship.

Pushed bb85017 for the three remaining review points:

• option_env!("RTK_TELEMETRY_URL").is_some() treated an empty URL as a configured endpoint. .github/workflows/release.yml passes the URL through an env: block fed by vars.RTK_TELEMETRY_URL, and an unset repository variable is exported as the empty string — so option_env! compiles to Some("") and a release that can never ping would still have blamed the salt. New utils::env_is_some covers the compile-time env check generally, and every RTK_TELEMETRY_URL read now goes through telemetry::endpoint_url, so the ping path, the status label and the erasure request can't drift apart.
• A present-but-malformed salt reported (no salt file yet; written on the first ping) in the one branch where run_status already knows the file exists. device_hash_line now has three arms — well-formed, malformed, absent.
• The len() == 64 guard was a byte check, so a 64-byte multi-byte string passed it and then panicked on &h[..8]. is_device_hash checks the alphabet too; there's a regression test for that exact input.

Verified on the rebased head: cargo fmt --all --check clean, cargo clippy --all-targets zero warnings, cargo test --all 2984 passed / 0 failed.