From 09e7716a1afde5591c2b692d667430681b5a1d43 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Mon, 6 Jul 2026 19:25:54 +0100 Subject: [PATCH 1/2] fix(cli): telemetry enable/disable fire cli_command_executed on pre-toggle consent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../telemetry/disable/SIDE_EFFECTS.md | 16 +++++++-- .../telemetry/disable/disable.command.ts | 11 +++++- .../commands/telemetry/enable/SIDE_EFFECTS.md | 17 +++++++-- .../telemetry/enable/enable.command.ts | 8 ++++- .../telemetry/telemetry.integration.test.ts | 36 ++++++++++++++++++- 5 files changed, 79 insertions(+), 9 deletions(-) diff --git a/apps/cli/src/legacy/commands/telemetry/disable/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/telemetry/disable/SIDE_EFFECTS.md index 1be0ffbf9f..fab9aabfe4 100644 --- a/apps/cli/src/legacy/commands/telemetry/disable/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/telemetry/disable/SIDE_EFFECTS.md @@ -17,7 +17,8 @@ instead of `~/.supabase/telemetry.json`. ## API Routes -`disable` is fully local. No network calls are made. +None called directly. `cli_command_executed` may be sent to PostHog — see +Telemetry Events Fired below. ## Environment Variables @@ -34,8 +35,17 @@ instead of `~/.supabase/telemetry.json`. ## Telemetry Events Fired -None. The command disables analytics capture so toggling telemetry does not emit -`cli_command_executed`, matching Go. +| Event | When | +| ---------------------- | ------------------------------------------------------------------------- | +| `cli_command_executed` | when telemetry was **already enabled** before this invocation (see Notes) | + +Go parity (`apps/cli-go/cmd/root.go:131-138,171-181`): the event is gated on +the consent state read at process start, before this command's handler +rewrites `telemetry.json` — not on the value the command just wrote. Running +`disable` while telemetry is enabled fires the event one last time (using +the pre-toggle, still-enabled snapshot); running it while telemetry is +already disabled stays silent. See `telemetry/enable/SIDE_EFFECTS.md` for +the mirror-image case. ## Output diff --git a/apps/cli/src/legacy/commands/telemetry/disable/disable.command.ts b/apps/cli/src/legacy/commands/telemetry/disable/disable.command.ts index 4914b2c2a6..4618901625 100644 --- a/apps/cli/src/legacy/commands/telemetry/disable/disable.command.ts +++ b/apps/cli/src/legacy/commands/telemetry/disable/disable.command.ts @@ -12,8 +12,17 @@ export const legacyTelemetryDisableCommand = Command.make("disable", config).pip Command.withDescription("Disable CLI telemetry."), Command.withShortDescription("Disable telemetry"), Command.withHandler((flags) => + // Go parity (`cmd/root.go:131-138,171-181`): `cli_command_executed` fires + // gated on the CONSENT SNAPSHOT TAKEN BEFORE this command's handler runs + // (Go's PersistentPreRunE reads the on-disk state before `disable`'s RunE + // mutates it), not on the just-written `false`. `legacyAnalyticsLayer` + // reproduces that naturally: it reads `TelemetryRuntime.consent` once, at + // layer-construction time, before the handler below ever executes, so + // leaving analytics enabled here fires the event exactly when telemetry + // was enabled prior to this invocation, and stays silent when it was + // already disabled — see `enable.command.ts` for the mirror-image case. legacyTelemetryDisable(flags).pipe( - withLegacyCommandInstrumentation({ analytics: false, flags }), + withLegacyCommandInstrumentation({ flags }), withJsonErrorHandling, ), ), diff --git a/apps/cli/src/legacy/commands/telemetry/enable/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/telemetry/enable/SIDE_EFFECTS.md index 3942a107dc..270de1584c 100644 --- a/apps/cli/src/legacy/commands/telemetry/enable/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/telemetry/enable/SIDE_EFFECTS.md @@ -17,7 +17,8 @@ instead of `~/.supabase/telemetry.json`. ## API Routes -`enable` is fully local. No network calls are made. +None called directly. `cli_command_executed` may be sent to PostHog — see +Telemetry Events Fired below. ## Environment Variables @@ -34,8 +35,18 @@ instead of `~/.supabase/telemetry.json`. ## Telemetry Events Fired -None. The command disables analytics capture so toggling telemetry does not emit -`cli_command_executed`, matching Go. +| Event | When | +| ---------------------- | ------------------------------------------------------------------------- | +| `cli_command_executed` | when telemetry was **already enabled** before this invocation (see Notes) | + +Go parity (`apps/cli-go/cmd/root.go:131-138,171-181`): the event is gated on +the consent state read at process start, before this command's handler +rewrites `telemetry.json` — not on the value the command just wrote. In the +common case (enabling from a disabled state) the pre-toggle snapshot is +`false`, so nothing fires; running `enable` while telemetry is already +enabled fires the event, matching Go's uniform, state-based (not +command-based) gate. See `telemetry/disable/SIDE_EFFECTS.md` for the +mirror-image case. ## Output diff --git a/apps/cli/src/legacy/commands/telemetry/enable/enable.command.ts b/apps/cli/src/legacy/commands/telemetry/enable/enable.command.ts index 5497f0e929..df79ad2505 100644 --- a/apps/cli/src/legacy/commands/telemetry/enable/enable.command.ts +++ b/apps/cli/src/legacy/commands/telemetry/enable/enable.command.ts @@ -12,8 +12,14 @@ export const legacyTelemetryEnableCommand = Command.make("enable", config).pipe( Command.withDescription("Enable CLI telemetry."), Command.withShortDescription("Enable telemetry"), Command.withHandler((flags) => + // Go parity (`cmd/root.go:131-138,171-181`): `cli_command_executed` fires + // gated on the pre-toggle consent snapshot, same as `disable` — see that + // command's comment. In the common case (enabling from a disabled state) + // the snapshot is `false`, so the event stays silent; running `enable` + // when telemetry is ALREADY enabled fires it, matching Go's uniform, + // state-based (not command-based) gate. legacyTelemetryEnable(flags).pipe( - withLegacyCommandInstrumentation({ analytics: false, flags }), + withLegacyCommandInstrumentation({ flags }), withJsonErrorHandling, ), ), diff --git a/apps/cli/src/legacy/commands/telemetry/telemetry.integration.test.ts b/apps/cli/src/legacy/commands/telemetry/telemetry.integration.test.ts index 99af77c201..34409088a6 100644 --- a/apps/cli/src/legacy/commands/telemetry/telemetry.integration.test.ts +++ b/apps/cli/src/legacy/commands/telemetry/telemetry.integration.test.ts @@ -8,6 +8,7 @@ import { Command } from "effect/unstable/cli"; import { mockAnalytics, mockOutput, processEnvLayer } from "../../../../tests/helpers/mocks.ts"; import { processControlLayer } from "../../../shared/runtime/process-control.layer.ts"; +import { EventCommandExecuted } from "../../../shared/telemetry/event-catalog.ts"; import { legacyTelemetryCommand } from "./telemetry.command.ts"; function makeTempDir(): string { @@ -32,7 +33,7 @@ function setup(dir: string) { processControlLayer, processEnvLayer({ SUPABASE_HOME: dir }), ); - return { out, layer }; + return { out, analytics, layer }; } function legacyTestRoot() { @@ -135,6 +136,39 @@ describe("legacy telemetry integration", () => { ) as Effect.Effect; }); + // Go parity (`cmd/root.go:131-138,171-181`): `cli_command_executed` is gated on + // the consent SNAPSHOT taken before the handler runs, not the value the handler + // just wrote — a property of Effect's layer-construction-once model, already + // covered generally by `legacyAnalyticsLayer`/`telemetryRuntimeLayer`'s own + // tests. These two assert the narrower wiring fix: `disable`/`enable` no longer + // force-suppress analytics via `analytics: false`, so the shared instrumentation + // wrapper actually reaches `Analytics.capture` for `cli_command_executed`. + it.live("disable no longer force-suppresses cli_command_executed", () => { + const dir = makeTempDir(); + const { analytics, layer } = setup(dir); + + return Effect.gen(function* () { + yield* Command.runWith(legacyTestRoot(), { version: "0.0.0-test" })(["telemetry", "disable"]); + expect(analytics.captured.map((event) => event.event)).toContain(EventCommandExecuted); + }).pipe( + Effect.provide(layer), + Effect.ensuring(Effect.sync(() => rmSync(dir, { recursive: true, force: true }))), + ) as Effect.Effect; + }); + + it.live("enable no longer force-suppresses cli_command_executed", () => { + const dir = makeTempDir(); + const { analytics, layer } = setup(dir); + + return Effect.gen(function* () { + yield* Command.runWith(legacyTestRoot(), { version: "0.0.0-test" })(["telemetry", "enable"]); + expect(analytics.captured.map((event) => event.event)).toContain(EventCommandExecuted); + }).pipe( + Effect.provide(layer), + Effect.ensuring(Effect.sync(() => rmSync(dir, { recursive: true, force: true }))), + ) as Effect.Effect; + }); + it.live( "status treats malformed typed fields as a corrupted file and regenerates identity", () => { From 0794bf9d6442ee63b82b2e87626197566f18f12a Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Mon, 6 Jul 2026 19:42:46 +0100 Subject: [PATCH 2/2] test(cli): exercise the real consent-gated analytics layer for telemetry toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../telemetry/disable/SIDE_EFFECTS.md | 11 +- .../commands/telemetry/enable/SIDE_EFFECTS.md | 11 +- .../telemetry/telemetry.integration.test.ts | 109 +++++++++++++++++- .../telemetry/runtime.layer.unit.test.ts | 46 ++++++++ 4 files changed, 169 insertions(+), 8 deletions(-) diff --git a/apps/cli/src/legacy/commands/telemetry/disable/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/telemetry/disable/SIDE_EFFECTS.md index fab9aabfe4..cb0719af81 100644 --- a/apps/cli/src/legacy/commands/telemetry/disable/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/telemetry/disable/SIDE_EFFECTS.md @@ -37,7 +37,7 @@ Telemetry Events Fired below. | Event | When | | ---------------------- | ------------------------------------------------------------------------- | -| `cli_command_executed` | when telemetry was **already enabled** before this invocation (see Notes) | +| `cli_command_executed` | when telemetry was **already enabled** before this invocation (see below) | Go parity (`apps/cli-go/cmd/root.go:131-138,171-181`): the event is gated on the consent state read at process start, before this command's handler @@ -47,6 +47,15 @@ the pre-toggle, still-enabled snapshot); running it while telemetry is already disabled stays silent. See `telemetry/enable/SIDE_EFFECTS.md` for the mirror-image case. +When the event fires, the process waits (bounded, up to ~5s) for a PostHog +flush attempt to complete or time out before exiting, since the analytics +client's shutdown is a finalizer around the whole command run. Previously +`disable`/`enable` never queued an event and returned immediately; on a +slow or fully offline network, this invocation can now take noticeably +longer than before, though the `Telemetry is disabled.` stdout line is +still written before that wait (it comes from the handler, which completes +before the surrounding instrumentation's post-run capture/flush step). + ## Output On success, every output mode writes the same raw stdout line: diff --git a/apps/cli/src/legacy/commands/telemetry/enable/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/telemetry/enable/SIDE_EFFECTS.md index 270de1584c..d9b81200ab 100644 --- a/apps/cli/src/legacy/commands/telemetry/enable/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/telemetry/enable/SIDE_EFFECTS.md @@ -37,7 +37,7 @@ Telemetry Events Fired below. | Event | When | | ---------------------- | ------------------------------------------------------------------------- | -| `cli_command_executed` | when telemetry was **already enabled** before this invocation (see Notes) | +| `cli_command_executed` | when telemetry was **already enabled** before this invocation (see below) | Go parity (`apps/cli-go/cmd/root.go:131-138,171-181`): the event is gated on the consent state read at process start, before this command's handler @@ -48,6 +48,15 @@ enabled fires the event, matching Go's uniform, state-based (not command-based) gate. See `telemetry/disable/SIDE_EFFECTS.md` for the mirror-image case. +When the event fires, the process waits (bounded, up to ~5s) for a PostHog +flush attempt to complete or time out before exiting, since the analytics +client's shutdown is a finalizer around the whole command run. Previously +`disable`/`enable` never queued an event and returned immediately; on a +slow or fully offline network, this invocation can now take noticeably +longer than before, though the `Telemetry is enabled.` stdout line is +still written before that wait (it comes from the handler, which completes +before the surrounding instrumentation's post-run capture/flush step). + ## Output On success, every output mode writes the same raw stdout line: diff --git a/apps/cli/src/legacy/commands/telemetry/telemetry.integration.test.ts b/apps/cli/src/legacy/commands/telemetry/telemetry.integration.test.ts index 34409088a6..870403c99e 100644 --- a/apps/cli/src/legacy/commands/telemetry/telemetry.integration.test.ts +++ b/apps/cli/src/legacy/commands/telemetry/telemetry.integration.test.ts @@ -6,9 +6,18 @@ import path from "node:path"; import { Effect, Layer } from "effect"; import { Command } from "effect/unstable/cli"; -import { mockAnalytics, mockOutput, processEnvLayer } from "../../../../tests/helpers/mocks.ts"; +import { + mockAnalytics, + mockOutput, + mockProjectContext, + mockRuntimeInfo, + mockTty, + processEnvLayer, +} from "../../../../tests/helpers/mocks.ts"; +import { cliConfigLayer } from "../../../next/config/cli-config.layer.ts"; import { processControlLayer } from "../../../shared/runtime/process-control.layer.ts"; import { EventCommandExecuted } from "../../../shared/telemetry/event-catalog.ts"; +import { legacyAnalyticsLayer } from "../../telemetry/legacy-analytics.layer.ts"; import { legacyTelemetryCommand } from "./telemetry.command.ts"; function makeTempDir(): string { @@ -36,6 +45,42 @@ function setup(dir: string) { return { out, analytics, layer }; } +// Wires the REAL `legacyAnalyticsLayer` (consent-gated, backed by +// `telemetryRuntimeLayer` reading `dir`'s telemetry.json) instead of +// `mockAnalytics()` — the un-mocked boundary `Analytics.capture` calls +// actually pass through. No PostHog key is set in the test env, so +// `legacyAnalyticsLayer` resolves to its no-op branch regardless of consent +// (real-network PostHog delivery has no test double anywhere in this repo); +// this proves the command runs the real consent-gated layer end-to-end +// without crashing, not the exact PostHog call count. The snapshot-timing +// mechanism itself (pre-toggle consent surviving the handler's own disk +// write) is proven directly in `runtime.layer.unit.test.ts`. +function setupWithRealAnalytics(dir: string) { + const out = mockOutput(); + const runtimeInfoLayer = mockRuntimeInfo({ homeDir: dir }); + const ttyLayer = mockTty(); + const envLayer = processEnvLayer({ SUPABASE_HOME: dir }); + const projectContextLayer = mockProjectContext(); + const configLayer = cliConfigLayer.pipe( + Layer.provide(runtimeInfoLayer), + Layer.provide(projectContextLayer), + ); + const analyticsLayer = legacyAnalyticsLayer.pipe( + Layer.provide(configLayer), + Layer.provide(runtimeInfoLayer), + Layer.provide(ttyLayer), + Layer.provide(BunServices.layer), + ); + const layer = Layer.mergeAll( + out.layer, + analyticsLayer, + BunServices.layer, + processControlLayer, + envLayer, + ); + return { out, layer }; +} + function legacyTestRoot() { return Command.make("supabase").pipe(Command.withSubcommands([legacyTelemetryCommand])); } @@ -138,11 +183,15 @@ describe("legacy telemetry integration", () => { // Go parity (`cmd/root.go:131-138,171-181`): `cli_command_executed` is gated on // the consent SNAPSHOT taken before the handler runs, not the value the handler - // just wrote — a property of Effect's layer-construction-once model, already - // covered generally by `legacyAnalyticsLayer`/`telemetryRuntimeLayer`'s own - // tests. These two assert the narrower wiring fix: `disable`/`enable` no longer - // force-suppress analytics via `analytics: false`, so the shared instrumentation - // wrapper actually reaches `Analytics.capture` for `cli_command_executed`. + // just wrote. These two assert the narrower wiring fix using `mockAnalytics()` + // (which unconditionally records every capture, bypassing consent entirely): + // `disable`/`enable` no longer force-suppress analytics via `analytics: false`, + // so the shared instrumentation wrapper actually reaches `Analytics.capture`. + // The snapshot-timing mechanism itself — that the pre-toggle value survives the + // handler's own on-disk write — is proven directly against `telemetryRuntimeLayer` + // in `shared/telemetry/runtime.layer.unit.test.ts`. The two tests further below + // run the same commands through the REAL, consent-gated `legacyAnalyticsLayer` + // (not this mock) to prove the production wiring doesn't crash end-to-end. it.live("disable no longer force-suppresses cli_command_executed", () => { const dir = makeTempDir(); const { analytics, layer } = setup(dir); @@ -169,6 +218,54 @@ describe("legacy telemetry integration", () => { ) as Effect.Effect; }); + it.live("disable runs cleanly through the real consent-gated analytics layer", () => { + const dir = makeTempDir(); + writeFileSync( + telemetryPath(dir), + JSON.stringify({ + enabled: true, + device_id: "device-123", + session_id: "session-123", + session_last_active: "2026-01-01T00:00:00.000Z", + schema_version: 1, + }), + ); + const { out, layer } = setupWithRealAnalytics(dir); + + return Effect.gen(function* () { + yield* Command.runWith(legacyTestRoot(), { version: "0.0.0-test" })(["telemetry", "disable"]); + expect(out.stdoutText).toBe("Telemetry is disabled.\n"); + expect(readTelemetryConfig(dir).enabled).toBe(false); + }).pipe( + Effect.provide(layer), + Effect.ensuring(Effect.sync(() => rmSync(dir, { recursive: true, force: true }))), + ) as Effect.Effect; + }); + + it.live("enable runs cleanly through the real consent-gated analytics layer", () => { + const dir = makeTempDir(); + writeFileSync( + telemetryPath(dir), + JSON.stringify({ + enabled: false, + device_id: "device-123", + session_id: "session-123", + session_last_active: "2026-01-01T00:00:00.000Z", + schema_version: 1, + }), + ); + const { out, layer } = setupWithRealAnalytics(dir); + + return Effect.gen(function* () { + yield* Command.runWith(legacyTestRoot(), { version: "0.0.0-test" })(["telemetry", "enable"]); + expect(out.stdoutText).toBe("Telemetry is enabled.\n"); + expect(readTelemetryConfig(dir).enabled).toBe(true); + }).pipe( + Effect.provide(layer), + Effect.ensuring(Effect.sync(() => rmSync(dir, { recursive: true, force: true }))), + ) as Effect.Effect; + }); + it.live( "status treats malformed typed fields as a corrupted file and regenerates identity", () => { diff --git a/apps/cli/src/shared/telemetry/runtime.layer.unit.test.ts b/apps/cli/src/shared/telemetry/runtime.layer.unit.test.ts index fbb165b4fa..a81bc3c044 100644 --- a/apps/cli/src/shared/telemetry/runtime.layer.unit.test.ts +++ b/apps/cli/src/shared/telemetry/runtime.layer.unit.test.ts @@ -138,4 +138,50 @@ describe("telemetryRuntimeLayer", () => { Effect.ensuring(Effect.sync(() => rmSync(homeDir, { recursive: true, force: true }))), ); }); + + // CLI-1868 (telemetry enable/disable firing cli_command_executed on pre-toggle + // consent) depends on this exact property: `consent` is read from disk once + // at layer-construction time and does not reflect a later on-disk write — + // mirroring Go's PersistentPreRunE snapshot, which a command's own RunE + // (e.g. `telemetry disable`'s SetEnabled) cannot retroactively change. + it.live("captures consent once; a later on-disk write does not change it", () => { + const homeDir = makeTempDir(); + const configPath = path.join(homeDir, "telemetry.json"); + writeFileSync( + configPath, + JSON.stringify({ + enabled: true, + device_id: "device-123", + session_id: "session-123", + session_last_active: "2026-04-01T12:00:00Z", + schema_version: 1, + }), + ); + + return Effect.gen(function* () { + const runtime = yield* TelemetryRuntime; + expect(runtime.consent).toBe("granted"); + + // Simulates `disable`'s handler rewriting the file mid-command, after + // this layer already resolved `consent` — the already-built runtime + // must keep reporting the pre-toggle value. + yield* Effect.sync(() => + writeFileSync( + configPath, + JSON.stringify({ + enabled: false, + device_id: "device-123", + session_id: "session-123", + session_last_active: "2026-04-01T12:00:00Z", + schema_version: 1, + }), + ), + ); + + expect(runtime.consent).toBe("granted"); + }).pipe( + Effect.provide(buildLayer({ homeDir })), + Effect.ensuring(Effect.sync(() => rmSync(homeDir, { recursive: true, force: true }))), + ); + }); });