fix(cli): telemetry enable/disable fire cli_command_executed on pre-toggle consent#5798
Merged
Coly010 merged 2 commits intoJul 7, 2026
Conversation
…oggle consent Go's cli_command_executed gate (cmd/root.go:131-138,171-181) reads the telemetry consent snapshot at process start, before the command's own handler runs — not the value it just wrote. TS unconditionally suppressed the event on both telemetry enable and disable via analytics: false, so neither ever fired. Removing analytics: false from both commands lets the existing legacyAnalyticsLayer/TelemetryRuntime machinery reproduce this correctly: those layers are built once, before the handler mutates telemetry.json, so whichever command runs fires the event exactly when telemetry was already enabled beforehand and stays silent otherwise — matching Go's uniform, state-based gate for all four enable/disable x already-enabled/disabled combinations, not just the "disable only" framing in the original report. Fixes CLI-1868.
…try toggle Review (architect/engineer/security/supabase-dx) converged on one gap: the CLI-1868 regression tests only used mockAnalytics(), which unconditionally records every capture and never touches legacyAnalyticsLayer's actual consent gate — so they proved the wiring fix but not the pre-toggle-snapshot behavior itself. - Add a runtime.layer.unit.test.ts case proving TelemetryRuntime.consent is captured once and survives a later on-disk write to telemetry.json — the core mechanism CLI-1868 depends on. - Add two telemetry.integration.test.ts cases that run disable/enable through the real legacyAnalyticsLayer (not the mock), proving the production wiring completes cleanly end-to-end. - Correct the "(see Notes)" dangling doc cross-references and document the new bounded (~5s) PostHog flush wait the fix introduces on the event-fires path, previously unmentioned. Filed CLI-1895 (should next/ adopt this behavior too?) and CLI-1896 (global flag redaction gap made reachable by this fix) as separate follow-ups.
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@0794bf9d6442ee63b82b2e87626197566f18f12aPreview package for commit |
avallete
approved these changes
Jul 7, 2026
Coly010
deleted the
columferry/cli-1868-telemetry-disable-fire-cli_command_executed-with-pre-toggle
branch
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
2 participants
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 kind of change does this PR introduce?
Bug fix (Go parity).
What is the current behavior?
Go's
cli_command_executedtelemetry event is gated on a consent snapshot read once at process start (PersistentPreRunE,apps/cli-go/cmd/root.go:131-138), before a command's own handler runs, and fired after the handler completes (:171-181) based on that snapshot — not on any value the handler itself just wrote. This applies uniformly to every command, includingtelemetry enable/disablethemselves.The TS legacy port instead unconditionally suppressed this event for both
telemetry enableandtelemetry disableviaanalytics: false, so neither command ever fired it, regardless of prior telemetry state.What is the new behavior?
analytics: falseis removed from both commands. The existinglegacyAnalyticsLayer/telemetryRuntimeLayermachinery already reads consent from disk once, at Effect layer-construction time — before either handler mutatestelemetry.json— so this reproduces Go's snapshot semantics without any new bespoke logic:disablewhile telemetry is enabled fires the event one last time (using the pre-toggle, still-enabled snapshot); running it while already disabled stays silent.enablewhile telemetry is already enabled (a redundant/idempotent invocation) fires the event; the common case (enabling from disabled) stays silent.This is intentionally broader than the original report's "disable only" framing — tracing Go's actual gate (
internal/telemetry/service.go'scanSend()) showed it's symmetric and state-based, not command-based, so scoping the fix todisablealone would have left a real (if narrow) parity gap for the redundant-enablecase.Also documents a previously-unmentioned side effect: when the event fires, the command now waits (bounded, ~5s) for a PostHog flush attempt before exiting, since the analytics client's shutdown is a finalizer around the whole command run.
Closes CLI-1868