fix(cli): make telemetry opt-out durable

[miguel-heygen]

Summary

hyperframes telemetry disable now remains disabled after the command exits. Previously, the background update check could read the old config before its network request, then write that stale snapshot after the telemetry command and silently restore telemetryEnabled: true.

The update checker now merges only its metadata into a fresh config snapshot, and telemetry commands skip the updater entirely. Enable/disable also fail non-zero when persistence fails, while telemetry status reports the effective config or environment opt-out source instead of claiming telemetry is enabled under HYPERFRAMES_NO_TELEMETRY=1 or DO_NOT_TRACK=1.

Validation

• Full CLI suite: 165 files passed, 2,190 tests passed
• Focused regression suite: 20 tests passed
• CLI package typecheck passed
• Oxlint and Oxfmt passed
• Clean-HOME command flow persisted telemetryEnabled: false with no update metadata race

Closes #2851

[vanceingalls]

R1 adversarial review at afec3f1f.

Framing: fidelity to #2851

The issue reports three failure modes on hyperframes@0.7.78:

1. disable prints ✓ Telemetry disabled but writes no config file.
2. status reports enabled immediately after a successful disable.
3. status reports enabled when HYPERFRAMES_NO_TELEMETRY=1 is set.

This PR addresses all three:

• (1) → packages/cli/src/commands/telemetry.ts:33-38 — setTelemetryEnabled now checks writeConfig's return and calls failCommand() (which throws CliRuntimeError — verified in packages/cli/src/utils/commandResult.ts); the trailing "✓ Telemetry disabled" is therefore unreachable on a persistence failure. Root cause of the "no file after disable" symptom identified in the PR body as the background update-check racing the disable write — plausible and matches the observed Windows-11 repro (update-check writes stale enabled:true snapshot after disable's write lands).
• (2) → the same update-check race + readConfigFresh in runStatus (line 44) so status no longer serves the in-process cache.
• (3) → effectiveTelemetryStatus (lines 18-26) reflects env-var opt-out in the displayed status and adds a Source: line.

Fidelity grade: CORRECT / A-. The root cause is correctly identified, the fix is minimal, and test coverage includes the exact race (checkAcrossConcurrentConfigWrite in updateCheck.test.ts:152-198 — releases the fetch AFTER a concurrent enabled:false write and asserts the write survives).

Findings

P2 — enable/disable don't warn when env-var opt-out overrides — parity gap with the status fix

packages/cli/src/commands/telemetry.ts:28-41 (setTelemetryEnabled).

Defect. HYPERFRAMES_NO_TELEMETRY=1 hyperframes telemetry enable prints ✓ Telemetry enabled and writes telemetryEnabled: true to disk, but client.ts:29 gates emission on the env var first, so nothing is tracked. From the user's point of view, the CLI just claimed to enable telemetry while silently keeping it off.

Failure. This is symmetric to the status bug this PR just fixed: the reporter said "every signal available to the user says it did not take" — the ✓ Telemetry enabled message is the same class of misleading success signal, just in the opposite direction. Any user who sets HYPERFRAMES_NO_TELEMETRY=1 in their shell profile and later runs hyperframes telemetry enable interactively (e.g. to debug a support issue) sees a green checkmark that lies.

Fix. Reuse effectiveTelemetryStatus in setTelemetryEnabled: after the write succeeds, if source !== "config", print a note like note: HYPERFRAMES_NO_TELEMETRY=1 currently overrides — unset to activate (analogous when disabling with env var already set: note: already disabled via HYPERFRAMES_NO_TELEMETRY). The message body of #2851 explicitly names this trust surface: "so this is the control for account-linked data collection."

P2 — effectiveTelemetryStatus diverges from shouldTrack on dev-mode and unconfigured-API-key gates

packages/cli/src/commands/telemetry.ts:18-26 vs packages/cli/src/telemetry/client.ts:24-45.

shouldTrack() short-circuits false on isDevMode() and on !POSTHOG_API_KEY.startsWith("phc_"). effectiveTelemetryStatus checks neither. In a dev build hyperframes telemetry status will report Status: enabled, Source: config while shouldTrack() returns false and no events flow. Real users on shipped releases don't hit either branch, so this is P2, not P1 — but it re-creates the exact "status says one thing, the emitter does another" divergence this PR set out to close. Cheapest fix: have effectiveTelemetryStatus (or a new sibling) call into a shared helper that returns the same verdict shouldTrack uses, and add corresponding Source: dev_mode / Source: telemetry_disabled_build labels.

P3 / nit — env-var value parsing is strict === "1"

packages/cli/src/commands/telemetry.ts:19,22 and packages/cli/src/telemetry/client.ts:29.

HYPERFRAMES_NO_TELEMETRY=true, =yes, =1 (trailing whitespace) all silently fall through to opt-in. The DO_NOT_TRACK consortium spec at https://consoledonottrack.com/ says "set the DO_NOT_TRACK environment variable to 1 or true" — the =true shape is documented for that flag and this code rejects it. Pre-existing (not a regression from this PR), and the emitter (client.ts) uses the same strict check so the status faithfully mirrors reality — but a Rames-quality durability audit should note the mismatch with the DO_NOT_TRACK spec. Not blocking for this PR; worth a follow-up.

P3 / nit — cross-process race left unaddressed; "durable" is aspirational there

packages/cli/src/telemetry/config.ts:writeConfig — the atomic-rename doc notes "narrows, but does not eliminate, lost updates against a concurrent CLI process." Concretely: hyperframes telemetry disable in terminal A writes enabled:false; a hyperframes render in terminal B that had already populated its process cache with enabled:true will call incrementCommandCount at startup (cli.ts:230) using the cached snapshot and overwrite the fresh disk state with enabled:true. Window is a few ms, and this is out of scope for the reporter's single-terminal repro, but the PR title's word "durable" is aspirational for the multi-process case. Recommend either a doc note in docs/ or a follow-up to switch incrementCommandCount (and the other in-process cached-then-write sites in autoUpdate.ts:167-190, skillsUpdateCheck.ts:47-59, telemetry/feedback.ts:36-67) to the same readConfigFresh pattern the trial-counter and the telemetry command now use.

Nit — import placement

packages/cli/src/commands/telemetry.ts:11 — the readConfigFresh, writeConfig, CONFIG_PATH import sits AFTER the examples array at lines 6-10. Pre-existing style oddity (the diff only changes readConfig → readConfigFresh here), but while the file is open, hoist it up with the other top-level imports.

Standing standards lens (mechanical)

• Named exports only: ✓ (the defineCommand default export is citty-required, expected)
• import type for type-only imports: ✓ (type Example, type TelemetryStatus locally)
• Atomic file write: ✓ (config.ts:writeConfig uses pid-suffixed tmp + renameSync, called out in the review)
• Tests colocated with source: ✓
• No any: ✓ (test uses as never once for the citty-args cast — acceptable and narrowly scoped)
• Commit signing / Co-Authored-By: single commit afec3f1f authored by miguel-heygen, no Co-Authored-By trailer, no bot involvement. Envelope clean.

Verdict

COMMENT — no P0/P1. Solid, high-fidelity fix with a plausible root-cause narrative and thorough tests (including the concurrent-write race). The two P2s (enable/disable env-var-override parity, and effectiveTelemetryStatus/shouldTrack divergence) are worth folding in before shipping since they re-open the same "status lies about reality" surface the PR set out to close — a privacy control's whole value is that the user can trust what the CLI tells them. Overall grade: A-.

— Review by Via

[james-russo-rames-d-jusso]

Reviewed at afec3f1.

Fix is correct in shape and traces cleanly to the #2851 repro. Two independent protections are layered here:

1. cli.ts no longer fires the update check for the telemetry command (line 249) — kills the specific same-process race the issue reports: telemetry disable writes telemetryEnabled: false, then the still-in-flight update-check's post-network writeConfig(staleConfig) used to overwrite it back to true. This exclusion means the update check simply never runs alongside a telemetry command, so no snapshot to be stale with.
2. updateCheck.ts reads a fresh config after the network round-trip (line 95) and only overwrites two metadata fields — this protects every other config writer (incrementCommandCount, showTelemetryNotice, feedback tracking, the DE parallel-router trial) from the same stale-snapshot pattern, since a render command in another shell can now flip telemetry without being clobbered by that same shell's update check.

The new updateCheck.test.ts::checkAcrossConcurrentConfigWrite unit-tests the second protection well — a concurrent telemetryEnabled: false write during the fetch-await window survives the update-check's writeback.

Cross-cutting theme in the inlines: env-var value handling is narrower than either the sibling packages/studio/src/telemetry/client.ts or the community DO_NOT_TRACK convention. Since the PR extends === "1" to a new site (the status display), it's a good moment to centralize the value check so a HYPERFRAMES_NO_TELEMETRY=true (a plausible user attempt) doesn't get silently ignored at both the status surface and the emitter (packages/cli/src/telemetry/client.ts:29).

Out-of-diff note — corrupted-config recovery is fail-OPEN: packages/cli/src/telemetry/config.ts:194-199 catches JSON.parse failures and RESETS to DEFAULT_CONFIG — { telemetryEnabled: true, telemetryNoticeShown: false, ... }. The writeConfig docstring above cites this as motivation for atomic renames, which covers torn writes but not the other corruption paths (external editor, cross-machine copy, disk error). On a "make opt-out durable" PR the safer bias would be to keep the file, log to stderr, and refuse writes until resolved — or at minimum reset toward telemetryEnabled: false. Out of scope here, but flagging while the durability lens is on.

Nothing blocking, CI green so far. LGTM from my side once the env-var value handling gets a decision — harden into a shared helper, or explicitly document =1-only in the disclosure.

— Review by Rames D Jusso

[miguel-heygen]

enable/disable don't warn when env-var opt-out overrides

effectiveTelemetryStatus diverges from shouldTrack on dev-mode and unconfigured-API-key gates

Addressed at 552bbfc02: command status and the emitter now use one effective telemetry policy. enable/disable distinguish the stored preference from the effective state and explain env/dev/build overrides instead of printing a misleading success. Tests cover both command directions plus dev-mode and disabled-build sources.

env-var value parsing is strict === "1"

Addressed: CLI status + emitter accept 1|true|yes|on case-insensitively after trimming, and Studio's build-time opt-out uses the same set. Direct emitter tests pin the privacy boundary.

corrupted-config recovery is fail-OPEN

Addressed as an adjacent privacy invariant: recovery now defaults telemetryEnabled to false, persists that safe state, and has a regression proving corrupt JSON cannot silently re-enable collection.

cross-process race left unaddressed

The observation is correct, but not claimed closed here: atomic rename prevents torn reads while independent read-modify-write processes still lack a real cross-process lock. This PR removes the reported update-check stale write and freshens its post-network snapshot; solving generic multi-writer serialization is a separate config-architecture change rather than pretending more pre-write reads are atomic.

Validation on this head: CLI 2215 passed / 2 skipped; Studio 2974 passed / 18 todo; both package typechecks; repository lint; pre-commit Fallow/format/size/tracked-artifact gates all green.