diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index 092860ce3..bbfc4b55c 100644 --- a/packages/cli/src/commands/render.test.ts +++ b/packages/cli/src/commands/render.test.ts @@ -30,11 +30,14 @@ const configState = vi.hoisted( cache: Record | null; writeConfigCalls: Array>; failWrites: number; + /** Config write lands but the install-state mirror does not. */ + failMirrors: number; } => ({ disk: { telemetryEnabled: true, deParallelRouterTrialFired: true }, cache: null, writeConfigCalls: [], failWrites: 0, + failMirrors: 0, }), ); @@ -147,6 +150,22 @@ vi.mock("../telemetry/config.js", () => ({ configState.cache = { ...config }; return true; }), + // The breaker's safety path uses this rather than writeConfig, so it can + // see a mirror failure instead of having it collapsed into `true`. + writeConfigWithResult: vi.fn((config: Record) => { + configState.writeConfigCalls.push({ ...config }); + if (configState.failWrites > 0) { + configState.failWrites--; + return { ok: false, error: "mock write failure" }; + } + configState.disk = { ...config }; + configState.cache = { ...config }; + if (configState.failMirrors > 0) { + configState.failMirrors--; + return { ok: true, mirrored: false }; + } + return { ok: true }; + }), })); vi.mock("../telemetry/client.js", () => ({ @@ -216,6 +235,7 @@ describe("renderLocal browser GPU config", () => { configState.disk = { telemetryEnabled: true, deParallelRouterTrialFired: true }; configState.cache = null; configState.failWrites = 0; + configState.failMirrors = 0; configState.writeConfigCalls = []; trackingState.shouldTrack = true; trackingState.renderObservations = []; @@ -1050,6 +1070,33 @@ describe("renderLocal — DE parallel-router CLI trial", () => { expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined(); }); + // The config write landing is NOT enough: config.json is the copy a stale + // writer or a re-mint can erase, so a run that mirrored nothing has left the + // safety fact on the erasable store only. writeConfig() collapsed + // {ok:true, mirrored:false} to success and the loop stopped there. + it("retries when the install-state mirror fails even though config.json landed", async () => { + configState.disk = { + telemetryEnabled: true, + deParallelRouterTrialFired: false, + telemetryNoticeShown: true, + }; + configState.failMirrors = 1; // first attempt mirrors nothing + producerState.executeImpl = async (job) => { + job.perfSummary = { + resolution: { width: 100, height: 100 }, + drawElement: { parallelRouter: "reverted" }, + }; + }; + await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions); + + expect(configState.disk.deParallelRouterTrialFired).toBe(true); + // Two writes: the one whose mirror failed, then the retry that mirrored. + const firedWrites = configState.writeConfigCalls.filter( + (c) => c.deParallelRouterTrialFired === true, + ); + expect(firedWrites.length).toBeGreaterThanOrEqual(2); + }); + it("re-asserts the fired flag when the write is lost (concurrent clobber / transient failure), without re-counting the render", async () => { configState.disk = { telemetryEnabled: true, diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index d83274636..78fbd6567 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -66,6 +66,7 @@ import { readConfigFresh, recordRecentRender, writeConfig, + writeConfigWithResult, type HyperframesConfig, } from "../telemetry/config.js"; import { shouldTrack } from "../telemetry/client.js"; @@ -1253,13 +1254,23 @@ function resolveDeParallelRouterOutcome(job: RenderJob): string | undefined { */ function persistDeParallelRouterTrialFired(): boolean { const MAX_ATTEMPTS = 3; + let mirrored = false; for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt++) { const config = readConfigFresh(); - if (config.deParallelRouterTrialFired) return true; + // Both stores must carry the latch, not just config.json. Checking only + // the config let a run stop early after a failed mirror — and config.json + // is the copy a stale writer or a re-mint can erase, so the durable one + // is exactly the one that was missing. The read side merges install-state + // back in, so the two together are what make the trip survive. + if (config.deParallelRouterTrialFired && mirrored) return true; config.deParallelRouterTrialFired = true; - if (!writeConfig(config)) return false; + const result = writeConfigWithResult(config); + if (!result.ok) return false; + mirrored = result.mirrored !== false; + if (mirrored) return true; + // Config landed but the mirror did not — retry rather than report success. } - return Boolean(readConfigFresh().deParallelRouterTrialFired); + return false; } /** diff --git a/packages/cli/src/commands/telemetry.test.ts b/packages/cli/src/commands/telemetry.test.ts index 3fe615032..27e62fbc7 100644 --- a/packages/cli/src/commands/telemetry.test.ts +++ b/packages/cli/src/commands/telemetry.test.ts @@ -37,7 +37,9 @@ async function loadTelemetryCommand(options?: { vi.doMock("../utils/env.js", () => ({ isDevMode: () => options?.devMode ?? false, })); - vi.doMock("../telemetry/transport.js", () => ({ + // The key moved to a leaf module to break a config -> policy -> transport + // -> config import cycle; policy.ts reads it from there now. + vi.doMock("../telemetry/posthogKey.js", () => ({ POSTHOG_API_KEY: options?.apiKey ?? "phc_test", })); const module = await import("./telemetry.js"); diff --git a/packages/cli/src/server/studioServer.ts b/packages/cli/src/server/studioServer.ts index e026679c4..8b5e7c10a 100644 --- a/packages/cli/src/server/studioServer.ts +++ b/packages/cli/src/server/studioServer.ts @@ -18,7 +18,7 @@ import { import { VERSION as version } from "../version.js"; import { buildStudioHeadScriptsForHost, - isLoopbackHost, + identityAllowed, resolveCliTelemetryDistinctId, } from "./telemetryIdentity.js"; import { emitStudioRenderComplete, emitStudioRenderError } from "./studioRenderTelemetry.js"; @@ -668,7 +668,7 @@ export function createStudioServer(options: StudioServerOptions): StudioServer { // attacker's hostname) is refused. Same-origin Studio traffic always // presents the bound loopback host. app.get("/api/telemetry-identity", (c) => { - if (!isLoopbackHost(c.req.header("host"))) { + if (!identityAllowed(c.req.header("host"))) { return c.json({ error: "forbidden" }, 403); } return c.json({ distinctId: resolveCliTelemetryDistinctId() }); diff --git a/packages/cli/src/server/telemetryIdentity.test.ts b/packages/cli/src/server/telemetryIdentity.test.ts index 83ea864c1..169f9ceb2 100644 --- a/packages/cli/src/server/telemetryIdentity.test.ts +++ b/packages/cli/src/server/telemetryIdentity.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi, beforeEach } from "vitest"; +import { afterEach, describe, expect, it, vi, beforeEach } from "vitest"; // CLI → Studio telemetry identity seeding (Layer 1). Verifies the server only // hands the browser a distinct id when CLI telemetry is enabled, and passes @@ -26,6 +26,7 @@ const { buildStudioHeadScripts, isLoopbackHost, buildStudioHeadScriptsForHost, + identityAllowed, } = await import("./telemetryIdentity.js"); describe("resolveCliTelemetryDistinctId", () => { @@ -251,3 +252,49 @@ describe("buildStudioHeadScriptsForHost — Host split", () => { expect(buildStudioHeadScriptsForHost(ENV, "localhost")).toContain("__HF_STUDIO_ENV__"); }); }); + +describe("identityAllowed — loopback-bound vs explicitly LAN-bound", () => { + const original = process.env["HYPERFRAMES_PREVIEW_HOST"]; + + afterEach(() => { + if (original === undefined) delete process.env["HYPERFRAMES_PREVIEW_HOST"]; + else process.env["HYPERFRAMES_PREVIEW_HOST"] = original; + }); + + describe("loopback-bound (the default)", () => { + beforeEach(() => { + delete process.env["HYPERFRAMES_PREVIEW_HOST"]; + }); + + it.each(["localhost:5173", "127.0.0.1", "[::1]:3000"])("allows %s", (host) => { + expect(identityAllowed(host)).toBe(true); + }); + + // A rebinding page cannot forge Host, so it arrives carrying its own name. + it.each(["evil.example.com", "127.0.0.1.evil.com", "192.168.1.10:3000", undefined])( + "refuses %s", + (host) => { + expect(identityAllowed(host)).toBe(false); + }, + ); + }); + + describe("explicitly LAN-bound", () => { + beforeEach(() => { + process.env["HYPERFRAMES_PREVIEW_HOST"] = "0.0.0.0"; + }); + + // The mode this regressed: browsing your own LAN-exposed Studio lost the + // CLI stitch entirely, so the same human became two PostHog persons. + it.each(["0.0.0.0:3000", "192.168.1.10:3000", "my-dev-box.local:3000"])( + "allows %s once the operator opted into LAN exposure", + (host) => { + expect(identityAllowed(host)).toBe(true); + }, + ); + + it("still allows loopback in that mode", () => { + expect(identityAllowed("localhost:3000")).toBe(true); + }); + }); +}); diff --git a/packages/cli/src/server/telemetryIdentity.ts b/packages/cli/src/server/telemetryIdentity.ts index f8d027274..a518a8708 100644 --- a/packages/cli/src/server/telemetryIdentity.ts +++ b/packages/cli/src/server/telemetryIdentity.ts @@ -57,14 +57,17 @@ function resolveCliBucketSeed(): string | null { * Is this request's `Host` a loopback name the studio server could have been * reached on directly? * - * Guards the identity endpoint against DNS rebinding: an attacker-controlled - * page can resolve its own hostname to 127.0.0.1 and read the response as - * same-origin, but the request still carries THAT hostname in `Host`. Genuine - * same-origin Studio traffic always presents the bound loopback host. - * * A bare `[::1]`/`localhost`/dotted-quad check rather than a full parse: the * port is irrelevant (any port on loopback is us), and anything exotic enough * to miss here should be refused rather than guessed at. + * + * Scope, stated precisely: this is a **DNS-rebinding mitigation for browsers**, + * not access control. A browser sets `Host` from the URL it was given, so a + * page that rebinds its own hostname to 127.0.0.1 arrives carrying that + * hostname and is refused. A non-browser client sets `Host` to whatever it + * likes, so this stops nothing there — but on a loopback-bound server such a + * client is already local, and on a LAN-bound one it can read the project + * files through the unauthenticated studio API anyway. See `identityAllowed`. */ export function isLoopbackHost(host: string | undefined): boolean { if (!host) return false; @@ -175,6 +178,28 @@ export function buildStudioHeadScripts( * injection branch when `packages/studio/dist` happens to be built, which is * true locally and false in the CI test lane. */ +/** + * May this request receive the CLI's identity (distinct id + bucket seed)? + * + * Two regimes, because the server binds loopback by DEFAULT and exposes the + * LAN only when an operator sets `HYPERFRAMES_PREVIEW_HOST` (portUtils.ts, + * F-001): + * + * - **Loopback-bound (default).** Anything reaching us came via loopback, so + * the only interesting attacker is a rebinding browser page — which the + * Host check catches, because a browser cannot forge `Host`. + * - **Explicitly LAN-bound.** The operator opted into exposing this server, + * and the Host header is trivially forgeable by any non-browser client, so + * the check buys nothing. Withholding identity there only broke the + * CLI-to-Studio stitch for the supported mode: the user browses + * `http://0.0.0.0:3000` or the machine's LAN IP, `isLoopbackHost` says no, + * and Studio mints a second anonymous person for the same human. + */ +export function identityAllowed(host: string | undefined): boolean { + const lanBound = (process.env["HYPERFRAMES_PREVIEW_HOST"] ?? "").trim() !== ""; + return lanBound || isLoopbackHost(host); +} + export function buildStudioHeadScriptsForHost(envScript: string, host: string | undefined): string { - return buildStudioHeadScripts(envScript, { includeIdentity: isLoopbackHost(host) }); + return buildStudioHeadScripts(envScript, { includeIdentity: identityAllowed(host) }); } diff --git a/packages/cli/src/telemetry/canary.test.ts b/packages/cli/src/telemetry/canary.test.ts index 80582db91..4771c0c14 100644 --- a/packages/cli/src/telemetry/canary.test.ts +++ b/packages/cli/src/telemetry/canary.test.ts @@ -44,6 +44,17 @@ vi.mock("@hyperframes/core/canary-registry", async () => { owner: "t", sunsetAfter: "2099-01-01", }, + { + // A PARTIAL percentage. `exclude` and the unit-id check only apply + // below 100 — at 100 the guard is about to be deleted, so everyone + // must already be on it — so exclusion behaviour cannot be expressed + // against test-alpha. + name: "test-gamma", + percentage: 50, + description: "partial rollout", + owner: "t", + sunsetAfter: "2099-01-01", + }, { name: "test-beta", percentage: 0, @@ -61,6 +72,13 @@ vi.mock("@hyperframes/core/canary-registry", async () => { owner: "t", sunsetAfter: "2099-01-01", }, + { + name: "test-gamma", + percentage: 50, + description: "", + owner: "t", + sunsetAfter: "2099-01-01", + }, { name: "test-beta", percentage: 0, @@ -84,6 +102,7 @@ beforeEach(() => { policyState.runtimeOverride = null; delete process.env.HF_CANARY_TEST_ALPHA; delete process.env.HF_CANARY_TEST_BETA; + delete process.env.HF_CANARY_TEST_GAMMA; }); describe("telemetry opt-out is canary opt-out", () => { @@ -121,6 +140,7 @@ describe("telemetry opt-out is canary opt-out", () => { configState.telemetryEnabled = false; expect(canaryEventProperties()).toEqual({ "$feature/canary-test-alpha": "false", + "$feature/canary-test-gamma": "false", "$feature/canary-test-beta": "false", }); }); @@ -130,16 +150,18 @@ describe("bucketing unit", () => { it("buckets on the bucketSeed when present — the unit that survives config wipes", async () => { const { evaluateCanary } = await import("@hyperframes/core/canary"); configState.bucketSeed = "5f1c9d2e-0000-4000-8000-aaaaaaaaaaaa"; - const viaBinding = resolveCanary("test-alpha").bucket; + const viaBinding = resolveCanary("test-gamma").bucket; + // 50, not 100: at 100 evaluateCanary short-circuits before bucketing and + // reports no bucket at all, which would make this comparison vacuous. const bySeed = evaluateCanary({ - feature: "test-alpha", + feature: "test-gamma", unitId: configState.bucketSeed, - percentage: 100, + percentage: 50, }).bucket; const byId = evaluateCanary({ - feature: "test-alpha", + feature: "test-gamma", unitId: configState.anonymousId, - percentage: 100, + percentage: 50, }).bucket; expect(viaBinding).toBe(bySeed); // Only meaningful if the two units actually bucket differently. @@ -148,13 +170,15 @@ describe("bucketing unit", () => { it("falls back to the anonymousId when no seed exists (failed legacy backfill)", async () => { const { evaluateCanary } = await import("@hyperframes/core/canary"); - const viaBinding = resolveCanary("test-alpha").bucket; + const viaBinding = resolveCanary("test-gamma").bucket; const byId = evaluateCanary({ - feature: "test-alpha", + feature: "test-gamma", unitId: configState.anonymousId, - percentage: 100, + percentage: 50, }).bucket; expect(viaBinding).toBe(byId); + // Both undefined would satisfy toBe — assert a bucket was actually computed. + expect(viaBinding).toEqual(expect.any(Number)); }); }); @@ -178,16 +202,16 @@ describe("CLI canary binding", () => { it("excludes CI from percentage enrolment, but an override still reaches it", () => { systemState.is_ci = true; - expect(resolveCanary("test-alpha")).toMatchObject({ enabled: false, reason: "excluded" }); + expect(resolveCanary("test-gamma")).toMatchObject({ enabled: false, reason: "excluded" }); __resetCanaryCacheForTests(); - process.env.HF_CANARY_TEST_ALPHA = "on"; - expect(resolveCanary("test-alpha")).toMatchObject({ enabled: true, reason: "forced_on" }); + process.env.HF_CANARY_TEST_GAMMA = "on"; + expect(resolveCanary("test-gamma")).toMatchObject({ enabled: true, reason: "forced_on" }); }); it("fails closed when the install has no anonymousId", () => { configState.anonymousId = ""; - expect(resolveCanary("test-alpha")).toMatchObject({ enabled: false, reason: "no_unit_id" }); + expect(resolveCanary("test-gamma")).toMatchObject({ enabled: false, reason: "no_unit_id" }); }); it("memoizes so a decision cannot change mid-process", () => { @@ -202,6 +226,7 @@ describe("CLI canary binding", () => { it("emits PostHog flag-shaped properties for every registered canary", () => { expect(canaryEventProperties()).toEqual({ "$feature/canary-test-alpha": "true", + "$feature/canary-test-gamma": expect.stringMatching(/^(true|false)$/), "$feature/canary-test-beta": "false", }); diff --git a/packages/cli/src/telemetry/canary.ts b/packages/cli/src/telemetry/canary.ts index 3026f8034..f14de2e5b 100644 --- a/packages/cli/src/telemetry/canary.ts +++ b/packages/cli/src/telemetry/canary.ts @@ -58,9 +58,18 @@ function telemetryActive(): boolean { */ const decisions = new Map(); +// The memo is scoped to a telemetry posture, not to the process. Within one +// render nothing may change (a render that starts enrolled must finish +// enrolled), but `hyperframes preview` is a long-lived server that re-serves +// decisions on every page load — and it used to keep serving pre-opt-out +// decisions for hours after `hyperframes telemetry disable`, which is exactly +// what the docs promise cannot happen. +let decisionsTelemetryPosture: boolean | undefined; + /** Test-only: drop memoized decisions so cases don't leak into each other. */ export function __resetCanaryCacheForTests(): void { decisions.clear(); + decisionsTelemetryPosture = undefined; } /** The uncached decision. Split out so `resolveCanary` is purely the memo. */ @@ -105,6 +114,11 @@ function decideCanary(name: string): CanaryDecision { * rollout control, and a typo in one must never take down a render. */ export function resolveCanary(name: string): CanaryDecision { + const posture = telemetryActive(); + if (decisionsTelemetryPosture !== posture) { + decisions.clear(); + decisionsTelemetryPosture = posture; + } const cached = decisions.get(name); if (cached) return cached; diff --git a/packages/cli/src/telemetry/config.test.ts b/packages/cli/src/telemetry/config.test.ts index 09b42ff46..a5e767440 100644 --- a/packages/cli/src/telemetry/config.test.ts +++ b/packages/cli/src/telemetry/config.test.ts @@ -36,6 +36,14 @@ vi.mock("node:fs", () => ({ }), })); +// The backfill warning is suppressed under a telemetry runtime override (a +// dev build counts as one, which vitest is), so the posture is controlled +// explicitly rather than inherited from the test environment. +const policyState = { runtimeOverride: null as string | null }; +vi.mock("./policy.js", () => ({ + telemetryRuntimeOverride: () => policyState.runtimeOverride, +})); + // Derived here rather than exported from config.ts: the pre-move path is // frozen history, so pinning the literal is the point — an export would just // let a rename pass silently, and it has no non-test consumer. @@ -446,6 +454,7 @@ describe("seed backfill write failure is surfaced", () => { }); it("warns once when the backfilled seed cannot be persisted", async () => { + policyState.runtimeOverride = null; // A config predating bucketSeed, on an unwritable home directory. const seeded = { telemetryEnabled: true, anonymousId: "id", telemetryNoticeShown: true }; fsState.files.set(CONFIG_PATH, JSON.stringify(seeded)); @@ -553,3 +562,119 @@ describe("the breaker latch is authoritative from install-state", () => { }); }); }); + +describe("install-state authority — negative latch and seed", () => { + let readConfig: typeof import("./config.js").readConfig; + let readConfigFresh: typeof import("./config.js").readConfigFresh; + let CONFIG_PATH: typeof import("./config.js").CONFIG_PATH; + let STATE_PATH: typeof import("./config.js").STATE_PATH; + + beforeEach(async () => { + fsState.files.clear(); + vi.resetModules(); + ({ readConfig, readConfigFresh, CONFIG_PATH, STATE_PATH } = await import("./config.js")); + }); + + // Only `true` is monotonic across processes. Caching `false` froze a stale + // negative in a long-lived process (the preview server), so a breaker + // tripped by another process was never picked up. + it("re-reads a negative latch: false -> external trip -> stale config -> fresh read is true", () => { + fsState.files.set(STATE_PATH, JSON.stringify({ markerAt: "2026-07-28T00:00:00.000Z" })); + fsState.files.set( + CONFIG_PATH, + JSON.stringify({ telemetryEnabled: true, anonymousId: "id", bucketSeed: "seed" }), + ); + expect(readConfig().deParallelRouterTrialFired).toBeUndefined(); + + // Another process trips the breaker and mirrors it... + fsState.files.set( + STATE_PATH, + JSON.stringify({ markerAt: "2026-07-28T00:00:00.000Z", deParallelRouterTrialFired: true }), + ); + // ...and a stale writer rewrites config.json without the flag. + fsState.files.set( + CONFIG_PATH, + JSON.stringify({ telemetryEnabled: true, anonymousId: "id", bucketSeed: "seed" }), + ); + + expect(readConfigFresh().deParallelRouterTrialFired).toBe(true); + }); + + // The latch got read-side authority; the seed did not, so the two stores + // could hold different seeds indefinitely and a re-mint flipped every cohort. + it("prefers the install-state seed over a restored config.json seed", () => { + fsState.files.set( + STATE_PATH, + JSON.stringify({ markerAt: "2026-07-28T00:00:00.000Z", bucketSeed: "seed-B" }), + ); + fsState.files.set( + CONFIG_PATH, + JSON.stringify({ telemetryEnabled: true, anonymousId: "id", bucketSeed: "seed-A" }), + ); + expect(readConfig().bucketSeed).toBe("seed-B"); + }); + + // A corrupt file OUTSIDE ~/.hyperframes survived the documented reset and + // reported a predecessor forever. + it("deletes an unreadable pre-move state file instead of reporting it forever", () => { + fsState.files.set(LEGACY_STATE_PATH, "{truncated"); + const config = readConfig(); + expect(config.stateFileCorrupt).toBe(true); + expect(fsState.files.has(LEGACY_STATE_PATH), "legacy copy removed").toBe(false); + }); +}); + +describe("an unwritable config dir must not re-roll the seed", () => { + let readConfig: typeof import("./config.js").readConfig; + let readConfigFresh: typeof import("./config.js").readConfigFresh; + + beforeEach(async () => { + fsState.files.clear(); + policyState.runtimeOverride = null; + vi.resetModules(); + ({ readConfig, readConfigFresh } = await import("./config.js")); + }); + + // No config.json AND an unwritable dir: cachedConfig was never set, so the + // next read re-minted and the seed re-rolled — on every command, forever. + it("keeps one seed for the process when the initial write fails", async () => { + const fs = await import("node:fs"); + vi.mocked(fs.writeFileSync).mockImplementation(() => { + throw new Error("EACCES: permission denied"); + }); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const first = readConfig(); + const second = readConfig(); + expect(first.bucketSeed).toBeTruthy(); + expect(second.bucketSeed).toBe(first.bucketSeed); + expect(second.anonymousId).toBe(first.anonymousId); + // And the user is told, rather than churning silently. + expect(warn).toHaveBeenCalledTimes(1); + + warn.mockRestore(); + vi.mocked(fs.writeFileSync).mockImplementation((path, content) => { + fsState.files.set(String(path), String(content)); + }); + }); + + it("stays silent for an install that opted out of telemetry", async () => { + policyState.runtimeOverride = "HYPERFRAMES_NO_TELEMETRY"; + const fs = await import("node:fs"); + vi.mocked(fs.writeFileSync).mockImplementation(() => { + throw new Error("EACCES: permission denied"); + }); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + readConfigFresh(); + // The only consequence is unstable canary cohorts, and an opted-out + // install is never enrolled in one — so this line was pure noise in CI + // logs, on every invocation, unsilenceable. + expect(warn).not.toHaveBeenCalled(); + + warn.mockRestore(); + vi.mocked(fs.writeFileSync).mockImplementation((path, content) => { + fsState.files.set(String(path), String(content)); + }); + }); +}); diff --git a/packages/cli/src/telemetry/config.ts b/packages/cli/src/telemetry/config.ts index 30eb92451..df587663e 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -3,6 +3,7 @@ import { join } from "node:path"; import { homedir } from "node:os"; import { randomUUID } from "node:crypto"; import { normalizeErrorMessage } from "../utils/errorMessage.js"; +import { telemetryRuntimeOverride } from "./policy.js"; // --------------------------------------------------------------------------- // Config directory: ~/.hyperframes/ @@ -124,6 +125,12 @@ let seedBackfillWarned = false; function warnSeedBackfillFailed(error: string | undefined): void { if (seedBackfillWarned) return; seedBackfillWarned = true; + // Suppressed when the user has opted out of telemetry: the only consequence + // of the failed write is unstable CANARY cohorts, and an opted-out install + // is not enrolled in any. Printing anyway put an unsilenceable line into + // CI render logs on every invocation for a user who wants no telemetry at + // all. `hyperframes doctor` still surfaces it on demand. + if (telemetryRuntimeOverride() !== null) return; console.warn( `[hyperframes] Could not persist telemetry config${error ? `: ${error}` : ""}. ` + "Canary cohort assignment will not be stable across runs.", @@ -147,9 +154,12 @@ function backfillBucketSeed(config: HyperframesConfig): void { if (!write.ok) warnSeedBackfillFailed(write.error); } -// The latch is monotonic — once tripped it never untrips — so one read per -// process is enough, and readConfig is hot (every command, every render). -let latchFromStateFile: boolean | undefined; +// ONLY the positive is cached. The latch is monotonic across processes in one +// direction: once some process trips it, it stays tripped. A cached `false` +// is not monotonic — another process can trip the breaker while this one is +// alive, and a long-lived process (the preview/studio server) would then hold +// a stale negative for hours. So `true` short-circuits and `false` re-reads. +let latchedFiredSeen = false; /** * Is the breaker latched according to install-state? @@ -163,11 +173,22 @@ let latchFromStateFile: boolean | undefined; * to prefer the safety fact. */ function installStateLatchedFired(): boolean { - if (latchFromStateFile === undefined) { - const state = readInstallState(); - latchFromStateFile = isInstallState(state) && state.deParallelRouterTrialFired === true; - } - return latchFromStateFile; + if (latchedFiredSeen) return true; + const state = readInstallState(); + latchedFiredSeen = isInstallState(state) && state.deParallelRouterTrialFired === true; + return latchedFiredSeen; +} + +// Same shape as the latch memo, and same reason for existing: readConfig is +// hot. A seed never changes once recorded, so the positive is cacheable. +let seedFromStateFile: string | undefined; + +/** The seed install-state has recorded for this machine, if any. */ +function installStateSeed(): string | undefined { + if (seedFromStateFile !== undefined) return seedFromStateFile; + const state = readInstallState(); + if (isInstallState(state) && state.bucketSeed !== undefined) seedFromStateFile = state.bucketSeed; + return seedFromStateFile; } /** Narrow the parse result to a usable record. */ @@ -192,6 +213,12 @@ function readInstallState(): InstallState | InstallStateMiss { } const legacy = parseInstallState(LEGACY_STATE_FILE); if (!isInstallState(legacy)) { + // Unreadable legacy copy: nothing to migrate, so drop it here too. It + // used to survive, and since it lives OUTSIDE ~/.hyperframes it then + // reported predecessorFound/stateFileCorrupt forever on a machine the + // user had already reset with `rm -rf ~/.hyperframes` — poisoning the one + // metric this file exists to produce. + if (legacy === "corrupt") removeLegacyStateFile(); // Corruption at EITHER location still means this machine had an install. return current === "corrupt" || legacy === "corrupt" ? "corrupt" : "absent"; } @@ -281,7 +308,8 @@ function applyInstallState(config: HyperframesConfig, wantFired: boolean): void if (next !== null) writeInstallState(next); stateMarkerSynced = true; stateFiredSynced = wantFired || state?.deParallelRouterTrialFired === true; - if (wantFired) latchFromStateFile = true; + if (wantFired) latchedFiredSeen = true; + if (next?.bucketSeed !== undefined) seedFromStateFile = next.bucketSeed; } function syncInstallState(config: HyperframesConfig): boolean { @@ -296,6 +324,23 @@ function syncInstallState(config: HyperframesConfig): boolean { } } +/** + * Mint a fresh config, persist it, and cache it EVEN IF the write failed. + * + * Without the unconditional cache the next readConfig in the same process + * re-minted, re-rolling bucketSeed along with the id — and across processes an + * unwritable ~/.hyperframes (read-only mount, root-owned after a sudo run, + * full disk) meant a fresh cohort on every single command, which is exactly + * the unbounded cumulative exposure the seed exists to prevent. + */ +function mintAndCacheConfig(): HyperframesConfig { + const config = mintConfig(); + const write = writeConfigWithResult(config); + if (!write.ok) warnSeedBackfillFailed(write.error); + cachedConfig = { ...config }; + return { ...config }; +} + /** * Build a brand-new config for an install with no (readable) config file, * consulting the install-state file for what a previous install on this @@ -501,55 +546,83 @@ function parseRecentRenders(value: unknown): RecentRenderRecord[] | undefined { * Read the config file, creating it with defaults if it doesn't exist. * Returns a mutable copy — call `writeConfig()` to persist changes. */ +/** + * Materialize a parsed config object, applying defaults and the explicit + * type guards. Split out of readConfig purely for size — that function is + * otherwise one long object literal plus four control-flow branches. + */ +/** Fields that are pure passthrough — no default, no validation. */ +function passthroughFields(parsed: Partial): Partial { + return { + lastUpdateCheck: parsed.lastUpdateCheck, + latestVersion: parsed.latestVersion, + lastStalePinNoticeAt: parsed.lastStalePinNoticeAt, + pendingUpdate: parsed.pendingUpdate, + completedUpdate: parsed.completedUpdate, + lastSkillsCheck: parsed.lastSkillsCheck, + skillsUpdateAvailable: parsed.skillsUpdateAvailable, + skillsOutdatedCount: parsed.skillsOutdatedCount, + skillsMissingCount: parsed.skillsMissingCount, + skillsRemovedCount: parsed.skillsRemovedCount, + }; +} + +/** + * Fields that need an explicit type guard or a cross-store merge, split from + * the plain defaults so neither block is complex on its own. + */ +function guardedFields(parsed: Partial): Partial { + return { + // Explicit `=== true`/typeof-number checks rather than a truthy/nullish + // read — a hand-edited or corrupted config could plausibly carry a + // non-boolean/non-number JSON value (e.g. the STRING "false", which is + // truthy in JS) for these two fields specifically, since they're read + // with a bare truthy check at the call site (review finding). + // `|| installStateLatchedFired()` — a latch recorded in install-state + // wins over an untripped config.json, never the reverse. + deParallelRouterTrialFired: + parsed.deParallelRouterTrialFired === true || installStateLatchedFired() ? true : undefined, + deParallelRouterTrialRenderCount: + typeof parsed.deParallelRouterTrialRenderCount === "number" + ? parsed.deParallelRouterTrialRenderCount + : undefined, + predecessorFound: + typeof parsed.predecessorFound === "boolean" ? parsed.predecessorFound : undefined, + stateFileCorrupt: parsed.stateFileCorrupt === true ? true : undefined, + // Install-state wins, exactly as it does for the latch. Without this the + // two stores could hold different seeds indefinitely: nextInstallState + // is write-once (state's seed always survives), but the read took + // config.json's blindly — so restoring or syncing only config.json left + // the install bucketing on A while install-state kept B, and the next + // re-mint silently flipped every cohort at once. + bucketSeed: installStateSeed() ?? parseNonEmptyString(parsed.bucketSeed), + }; +} + +function materializeConfig(parsed: Partial): HyperframesConfig { + return { + ...passthroughFields(parsed), + telemetryEnabled: parsed.telemetryEnabled ?? DEFAULT_CONFIG.telemetryEnabled, + anonymousId: parsed.anonymousId || randomUUID(), + telemetryNoticeShown: parsed.telemetryNoticeShown ?? DEFAULT_CONFIG.telemetryNoticeShown, + commandCount: parsed.commandCount ?? DEFAULT_CONFIG.commandCount, + renderSuccessCount: parsed.renderSuccessCount ?? DEFAULT_CONFIG.renderSuccessCount, + lastFeedbackPromptAt: parsed.lastFeedbackPromptAt ?? DEFAULT_CONFIG.lastFeedbackPromptAt, + ...guardedFields(parsed), + recentRenders: parseRecentRenders(parsed.recentRenders), + }; +} + export function readConfig(): HyperframesConfig { if (cachedConfig) return { ...cachedConfig }; - if (!existsSync(CONFIG_FILE)) { - const config = mintConfig(); - writeConfig(config); - return config; - } + if (!existsSync(CONFIG_FILE)) return mintAndCacheConfig(); try { const raw = readFileSync(CONFIG_FILE, "utf-8"); const parsed = JSON.parse(raw) as Partial; - const config: HyperframesConfig = { - telemetryEnabled: parsed.telemetryEnabled ?? DEFAULT_CONFIG.telemetryEnabled, - anonymousId: parsed.anonymousId || randomUUID(), - telemetryNoticeShown: parsed.telemetryNoticeShown ?? DEFAULT_CONFIG.telemetryNoticeShown, - commandCount: parsed.commandCount ?? DEFAULT_CONFIG.commandCount, - renderSuccessCount: parsed.renderSuccessCount ?? DEFAULT_CONFIG.renderSuccessCount, - lastFeedbackPromptAt: parsed.lastFeedbackPromptAt ?? DEFAULT_CONFIG.lastFeedbackPromptAt, - lastUpdateCheck: parsed.lastUpdateCheck, - latestVersion: parsed.latestVersion, - lastStalePinNoticeAt: parsed.lastStalePinNoticeAt, - pendingUpdate: parsed.pendingUpdate, - completedUpdate: parsed.completedUpdate, - lastSkillsCheck: parsed.lastSkillsCheck, - skillsUpdateAvailable: parsed.skillsUpdateAvailable, - skillsOutdatedCount: parsed.skillsOutdatedCount, - skillsMissingCount: parsed.skillsMissingCount, - skillsRemovedCount: parsed.skillsRemovedCount, - // Explicit `=== true`/typeof-number checks rather than a truthy/nullish - // read — a hand-edited or corrupted config could plausibly carry a - // non-boolean/non-number JSON value (e.g. the STRING "false", which is - // truthy in JS) for these two fields specifically, since they're read - // with a bare truthy check at the call site (review finding). - // `|| installStateLatchedFired()` — a latch recorded in install-state - // wins over an untripped config.json, never the reverse. - deParallelRouterTrialFired: - parsed.deParallelRouterTrialFired === true || installStateLatchedFired() ? true : undefined, - deParallelRouterTrialRenderCount: - typeof parsed.deParallelRouterTrialRenderCount === "number" - ? parsed.deParallelRouterTrialRenderCount - : undefined, - predecessorFound: - typeof parsed.predecessorFound === "boolean" ? parsed.predecessorFound : undefined, - stateFileCorrupt: parsed.stateFileCorrupt === true ? true : undefined, - bucketSeed: parseNonEmptyString(parsed.bucketSeed), - recentRenders: parseRecentRenders(parsed.recentRenders), - }; + const config = materializeConfig(parsed); // One-time backfill for configs predating the bucket seed: prefer the // recorded seed if a previous install already wrote one, else mint. diff --git a/packages/cli/src/telemetry/policy.test.ts b/packages/cli/src/telemetry/policy.test.ts index 4894f29fc..4ffc93f29 100644 --- a/packages/cli/src/telemetry/policy.test.ts +++ b/packages/cli/src/telemetry/policy.test.ts @@ -5,7 +5,9 @@ async function loadPolicy(options?: { devMode?: boolean; apiKey?: string }) { vi.doMock("../utils/env.js", () => ({ isDevMode: () => options?.devMode ?? false, })); - vi.doMock("./transport.js", () => ({ + // The key moved to a leaf module to break a config -> policy -> transport + // -> config import cycle; policy.ts reads it from there now. + vi.doMock("./posthogKey.js", () => ({ POSTHOG_API_KEY: options?.apiKey ?? "phc_test", })); return import("./policy.js"); diff --git a/packages/cli/src/telemetry/policy.ts b/packages/cli/src/telemetry/policy.ts index 78e97df84..8daa7cfef 100644 --- a/packages/cli/src/telemetry/policy.ts +++ b/packages/cli/src/telemetry/policy.ts @@ -1,5 +1,5 @@ import { isDevMode } from "../utils/env.js"; -import { POSTHOG_API_KEY } from "./transport.js"; +import { POSTHOG_API_KEY } from "./posthogKey.js"; export type TelemetryStatusSource = | "config" diff --git a/packages/cli/src/telemetry/posthogKey.ts b/packages/cli/src/telemetry/posthogKey.ts new file mode 100644 index 000000000..fe72aa24a --- /dev/null +++ b/packages/cli/src/telemetry/posthogKey.ts @@ -0,0 +1,10 @@ +/** + * The PostHog write-only ingest key, as a LEAF module. + * + * It lives here rather than in transport.ts because `policy.ts` needs it (an + * unconfigured key is itself a telemetry opt-out) and importing transport for + * one constant created `config.ts -> policy.ts -> transport.ts -> config.ts`. + * A cycle through the telemetry config is the kind that bites at module-init + * time, so the constant moved instead of the dependency being tolerated. + */ +export const POSTHOG_API_KEY = "phc_zjjbX0PnWxERXrMHhkEJWj9A9BhGVLRReICgsfTMmpx"; diff --git a/packages/cli/src/telemetry/transport.ts b/packages/cli/src/telemetry/transport.ts index 9ec9ac003..42fe4971c 100644 --- a/packages/cli/src/telemetry/transport.ts +++ b/packages/cli/src/telemetry/transport.ts @@ -1,10 +1,11 @@ import { spawn } from "node:child_process"; import { randomUUID } from "node:crypto"; +import { POSTHOG_API_KEY } from "./posthogKey.js"; import { readConfig } from "./config.js"; // This is a public project API key — safe to embed in client-side code. // It only allows writing events, not reading data. -export const POSTHOG_API_KEY = "phc_zjjbX0PnWxERXrMHhkEJWj9A9BhGVLRReICgsfTMmpx"; + const POSTHOG_HOST = "https://us.i.posthog.com"; const FLUSH_TIMEOUT_MS = 5_000; diff --git a/packages/core/src/canary.test.ts b/packages/core/src/canary.test.ts index f80cb2fa2..03da853ca 100644 --- a/packages/core/src/canary.test.ts +++ b/packages/core/src/canary.test.ts @@ -1,5 +1,4 @@ import { describe, expect, it } from "vitest"; -import { randomUUID } from "node:crypto"; import { canaryBucket, evaluateCanary, parseCanaryOverride, type CanaryInput } from "./canary.js"; import { CANARIES, canaryEnvVar, findCanary, overdueCanaries } from "./canaryRegistry.js"; import { CANARY_FEATURE_PREFIX, canaryFeatureKey, canaryFeatureProperties } from "./canary.js"; @@ -27,9 +26,36 @@ function rawFnv(input: string): number { return hash >>> 0; } -/** A realistic population: install ids are v4 UUIDs (`randomUUID()`). */ -function uuids(n: number): string[] { - return Array.from({ length: n }, () => randomUUID()); +/** + * A realistic population: v4-shaped UUIDs, but from a SEEDED PRNG. + * + * These ids feed statistical assertions (share within 1pp, chi-square + * uniformity) whose thresholds are tight enough to fail by chance on a + * genuinely random draw: measured at ~4 failures per 1500 runs for the share + * bound (the pct=50 case has a binomial SD of 0.354pp, so 1pp is only 2.8 + * sigma) and 1 per 1000 for chi-square by its own construction. That made the + * whole @hyperframes/core suite flaky for unrelated PRs. Seeded means the + * population is fixed, so a failure is a real change in the hash — which is + * the only thing these tests are for. + */ +function uuids(n: number, seed = 0x9e3779b9): string[] { + let state = seed >>> 0; + const nextByte = (): number => { + // xorshift32 — deterministic, and uniform enough to stand in for a real + // id population. Not used for anything security-relevant. + state ^= state << 13; + state >>>= 0; + state ^= state >>> 17; + state ^= state << 5; + state >>>= 0; + return state & 0xff; + }; + const hex = (count: number): string => + Array.from({ length: count }, () => nextByte().toString(16).padStart(2, "0")).join(""); + return Array.from( + { length: n }, + () => `${hex(4)}-${hex(2)}-4${hex(2).slice(1)}-a${hex(2).slice(1)}-${hex(6)}`, + ); } describe("fnv1a32 (via canaryBucket)", () => { @@ -87,9 +113,9 @@ describe("evaluateCanary", () => { } }); - it("fails closed without a unit id — unknown must never mean everyone", () => { + it("fails closed without a unit id — unknown must never mean a PARTIAL cohort", () => { for (const id of [undefined, "", " "]) { - expect(evaluateCanary(base({ unitId: id, percentage: 100 }))).toEqual({ + expect(evaluateCanary(base({ unitId: id, percentage: 50 }))).toEqual({ enabled: false, reason: "no_unit_id", }); @@ -97,12 +123,32 @@ describe("evaluateCanary", () => { }); it("excludes flagged units (CI) from percentage enrolment but not from an override", () => { - expect(evaluateCanary(base({ percentage: 100, exclude: true })).reason).toBe("excluded"); - expect(evaluateCanary(base({ percentage: 100, exclude: true, override: true })).enabled).toBe( + expect(evaluateCanary(base({ percentage: 50, exclude: true })).reason).toBe("excluded"); + expect(evaluateCanary(base({ percentage: 50, exclude: true, override: true })).enabled).toBe( true, ); }); + // 100 is the one percentage where "we don't know who this is" and "this is + // CI" stop mattering: the registry's step 4 says to delete the entry and the + // guard at 100-and-holding, so any population still resolving false here + // would take the new path for the FIRST time at deletion — unstaged, and + // invisible on the dashboard that said it was safe. + it.each([ + ["no unit id", { unitId: undefined }], + ["blank unit id", { unitId: " " }], + ["excluded (CI)", { exclude: true }], + ])("at 100%% enrols %s, so deleting the guard changes nothing", (_label, extra) => { + expect(evaluateCanary(base({ percentage: 100, ...extra }))).toEqual({ + enabled: true, + reason: "in_cohort", + }); + }); + + it("an explicit off still wins at 100%", () => { + expect(evaluateCanary(base({ percentage: 100, override: false })).enabled).toBe(false); + }); + it("clamps out-of-range and fractional percentages", () => { expect(evaluateCanary(base({ percentage: -5 })).enabled).toBe(false); expect(evaluateCanary(base({ percentage: 999 })).enabled).toBe(true); @@ -264,10 +310,29 @@ describe("registry", () => { expect(findCanary("nope")).toBeUndefined(); }); - it("no canary is past its sunset date", () => { - // Fails the suite when a rollout has been left half-finished. Either take - // it to 100 and delete the entry, or move the date deliberately. - expect(overdueCanaries()).toEqual([]); + // Deliberately NOT `overdueCanaries()` with the ambient date. That assertion + // reads wall-clock time, so it turns the entire @hyperframes/core suite red + // on a calendar date for every unrelated PR — a broken build nobody caused + // and whose fix is unrelated to the change under test. The registry's own + // freshness is enforced by the pinned dates below plus the sunset REPORT, + // which is advisory rather than a gate. + it("every canary carries a parseable sunset date in the future at authoring time", () => { + const authored = new Date("2026-07-31T00:00:00Z"); + for (const c of CANARIES) { + const sunset = Date.parse(`${c.sunsetAfter}T00:00:00Z`); + expect(Number.isFinite(sunset), `${c.name} has an unparseable sunsetAfter`).toBe(true); + expect(sunset, `${c.name} was authored already-expired`).toBeGreaterThan(authored.getTime()); + } + }); + + it("reports a canary as overdue only AFTER the whole sunset day has passed", () => { + const [first] = CANARIES; + if (!first) return; + const day = first.sunsetAfter; + expect(overdueCanaries(new Date(`${day}T00:00:00Z`))).not.toContain(first.name); + expect(overdueCanaries(new Date(`${day}T23:59:59Z`))).not.toContain(first.name); + const dayAfter = new Date(Date.parse(`${day}T00:00:00Z`) + 86_400_000); + expect(overdueCanaries(dayAfter)).toContain(first.name); }); }); diff --git a/packages/core/src/canary.ts b/packages/core/src/canary.ts index cf8d40fb4..5fb5cd6f3 100644 --- a/packages/core/src/canary.ts +++ b/packages/core/src/canary.ts @@ -119,14 +119,18 @@ export function evaluateCanary(input: CanaryInput): CanaryDecision { const pct = Math.max(0, Math.min(100, Math.trunc(input.percentage))); if (pct <= 0) return { enabled: false, reason: "out_of_cohort" }; + // Ahead of `exclude` and the unit-id check: at 100 the registry's step 4 + // says to delete the entry and the guard, so anything still resolving false + // here would take the new path for the FIRST time at deletion, unstaged. + // CI and seedless installs are exactly the populations a dashboard cannot + // see, so "100% and holding" looked green while they were never exercised. + if (pct >= 100) return { enabled: true, reason: "in_cohort" }; + if (input.exclude) return { enabled: false, reason: "excluded" }; const unitId = input.unitId?.trim(); if (!unitId) return { enabled: false, reason: "no_unit_id" }; - if (pct >= 100) - return { enabled: true, reason: "in_cohort", bucket: canaryBucket(input.feature, unitId) }; - const bucket = canaryBucket(input.feature, unitId); return bucket < pct ? { enabled: true, reason: "in_cohort", bucket } diff --git a/packages/core/src/canaryRegistry.ts b/packages/core/src/canaryRegistry.ts index d74a2b139..f0ad7936c 100644 --- a/packages/core/src/canaryRegistry.ts +++ b/packages/core/src/canaryRegistry.ts @@ -106,7 +106,10 @@ export function canaryEnvVar(name: string): string { */ export function overdueCanaries(now: Date = new Date()): string[] { return CANARIES.filter((c) => { - const sunset = Date.parse(`${c.sunsetAfter}T00:00:00Z`); - return Number.isFinite(sunset) && now.getTime() > sunset; + // End of the sunset day, not its start. The field is documented as the + // date AFTER which a canary is overdue, but comparing against midnight UTC + // made it overdue ON that date — and earlier still for anyone west of UTC. + const sunsetEnd = Date.parse(`${c.sunsetAfter}T23:59:59.999Z`); + return Number.isFinite(sunsetEnd) && now.getTime() > sunsetEnd; }).map((c) => c.name); } diff --git a/packages/studio/src/telemetry/canary.ts b/packages/studio/src/telemetry/canary.ts index 8c90f8088..d6240b2b7 100644 --- a/packages/studio/src/telemetry/canary.ts +++ b/packages/studio/src/telemetry/canary.ts @@ -213,14 +213,25 @@ function decideStudioCanary(name: string): CanaryDecision { const override = readOverride(definition.name); if (override === undefined) { if (!browserTelemetryAllowed()) return { enabled: false, reason: "telemetry_opt_out" }; + // Studio's exclusion is its own to apply: the CLI cannot see + // navigator.webdriver, so adopting its cohort decision verbatim enrolled + // Playwright/Puppeteer sessions driving a local `hyperframes preview` — + // each minting a fresh localStorage id, precisely the ephemeral-id noise + // the exclusion exists to keep out of the rollout signal. + if (isAutomatedBrowser()) return { enabled: false, reason: "excluded" }; if (fromCli !== undefined) return cohortOutcome(fromCli.enabled); } + // An explicit override decides on evaluateCanary's first line without ever + // reading unitId, so resolving the unit here would mint and PERSIST an + // anonymous id purely as an unused argument — for a profile that may have + // opted out. One click on a support link created a durable tracking id. + if (override !== undefined) return forcedOutcome(override); + return evaluateCanary({ feature: definition.name, unitId: resolveBucketUnit(), percentage: definition.percentage, - override, exclude: isAutomatedBrowser(), }); } diff --git a/packages/studio/src/telemetry/client.test.ts b/packages/studio/src/telemetry/client.test.ts index 079bbe496..85602bb74 100644 --- a/packages/studio/src/telemetry/client.test.ts +++ b/packages/studio/src/telemetry/client.test.ts @@ -77,10 +77,15 @@ describe("studio client shouldTrack", () => { expect(shouldTrack()).toBe(false); }); - it("memoizes its decision after the first call", async () => { + // Previously asserted the opposite. That memoization WAS the bug: policy.ts + // is explicit that transports re-ask, and policy.test.ts asserts a + // mid-session opt-out takes effect at once — but this transport cached on + // first call, so a user who opted out in DevTools after one event kept + // sending `studio_*` and render events while `studio:*` correctly stopped. + it("re-reads the policy, so a mid-session opt-out takes effect immediately", async () => { const shouldTrack = await loadShouldTrack(); - const first = shouldTrack(); + expect(shouldTrack()).toBe(true); localStorage.setItem(OPT_OUT_KEY, "1"); - expect(shouldTrack()).toBe(first); + expect(shouldTrack()).toBe(false); }); }); diff --git a/packages/studio/src/telemetry/client.ts b/packages/studio/src/telemetry/client.ts index 511571bb4..615d497d3 100644 --- a/packages/studio/src/telemetry/client.ts +++ b/packages/studio/src/telemetry/client.ts @@ -24,14 +24,14 @@ interface QueuedEvent { let eventQueue: QueuedEvent[] = []; let flushTimer: ReturnType | null = null; -let telemetryEnabled: boolean | null = null; export function shouldTrack(): boolean { - if (telemetryEnabled !== null) return telemetryEnabled; - // Delegated to telemetry/policy.ts so this transport, the older `studio:*` - // transport, and canary enrolment cannot drift apart again. - telemetryEnabled = browserTelemetryAllowed(); - return telemetryEnabled; + // NOT memoized. policy.ts is explicit that the transports re-ask, and + // policy.test.ts asserts a mid-session opt-out takes effect at once — but + // this cached on first call, so a user who opted out in DevTools after one + // event kept sending `studio_*` and render events for the rest of the tab + // while `studio:*` correctly stopped. The check is two property reads. + return browserTelemetryAllowed(); } export function trackEvent(event: string, properties: EventProperties = {}): void { diff --git a/packages/studio/src/telemetry/config.ts b/packages/studio/src/telemetry/config.ts index f4326b97b..970f5b136 100644 --- a/packages/studio/src/telemetry/config.ts +++ b/packages/studio/src/telemetry/config.ts @@ -22,12 +22,26 @@ export function getAnonymousId(): string { return resolveStudioDistinctId(); } +// safeLocalStorage() guards the REFERENCE, not the access: in a partitioned +// or sandboxed context the object resolves and `getItem` still throws (the +// case distinctId.ts already documents). These are read from the telemetry +// policy, which is called from event tracking that must never throw into a +// caller — `trackStudioEvent` sits in a post-commit catch block, so a throw +// there reported an already-committed edit as failed. +function readStoredFlag(key: string): boolean { + try { + return safeLocalStorage()?.getItem(key) === "1"; + } catch { + return false; + } +} + export function isOptedOut(): boolean { - return safeLocalStorage()?.getItem(OPT_OUT_KEY) === "1"; + return readStoredFlag(OPT_OUT_KEY); } export function hasShownNotice(): boolean { - return safeLocalStorage()?.getItem(NOTICE_KEY) === "1"; + return readStoredFlag(NOTICE_KEY); } export function markNoticeShown(): void { diff --git a/packages/studio/src/telemetry/distinctId.test.ts b/packages/studio/src/telemetry/distinctId.test.ts index b42011463..b6e77119b 100644 --- a/packages/studio/src/telemetry/distinctId.test.ts +++ b/packages/studio/src/telemetry/distinctId.test.ts @@ -101,3 +101,38 @@ describe("getCliDistinctId", () => { expect(getCliDistinctId()).toBeNull(); }); }); + +describe("no-storage fallback must not collapse the population", () => { + const realLocalStorage = Object.getOwnPropertyDescriptor(globalThis, "localStorage"); + + beforeEach(() => { + __resetStudioDistinctIdForTests(); + // Simulate a hardened / partitioned context where safeLocalStorage() + // returns null. + Object.defineProperty(globalThis, "localStorage", { value: undefined, configurable: true }); + }); + + afterEach(() => { + if (realLocalStorage) Object.defineProperty(globalThis, "localStorage", realLocalStorage); + __resetStudioDistinctIdForTests(); + }); + + it("is stable within a session", () => { + const first = resolveStudioDistinctId(); + expect(resolveStudioDistinctId()).toBe(first); + expect(first).toBeTruthy(); + }); + + // Regression: this used to return the shared literal "anonymous", so every + // storage-restricted profile was ONE bucketing unit. Against the shipped + // hash `calibration-50:anonymous` lands in bucket 44, so that whole + // population was enrolled at a nominal 50% and would flip together on a + // ramp — and they all merged into one PostHog person. + it("differs across sessions rather than sharing one constant", () => { + const first = resolveStudioDistinctId(); + __resetStudioDistinctIdForTests(); + const second = resolveStudioDistinctId(); + expect(second).not.toBe(first); + expect(first).not.toBe("anonymous"); + }); +}); diff --git a/packages/studio/src/telemetry/distinctId.ts b/packages/studio/src/telemetry/distinctId.ts index 241b9225a..790beaf88 100644 --- a/packages/studio/src/telemetry/distinctId.ts +++ b/packages/studio/src/telemetry/distinctId.ts @@ -106,9 +106,18 @@ export function resolveStudioDistinctId(): string { return existing; } } else { - // No storage at all (SSR / locked-down browser): stable within the session. + // No storage at all (SSR / locked-down browser): stable within the + // session, but RANDOM per session rather than a shared literal. + // + // It used to be the constant "anonymous", which made every + // storage-restricted profile one bucketing unit: computed against the + // shipped hash, `calibration-50:anonymous` lands in bucket 44, so 100% of + // that population was enrolled at a nominal 50% and would flip together + // on any ramp. It also merged unrelated users into a single PostHog + // person. A per-session id spreads them and keeps them distinct, while + // persisting nothing. // `cachedId` is guaranteed null here (early-returned at the top otherwise). - cachedId = "anonymous"; + cachedId = generateId(); return cachedId; } diff --git a/packages/studio/src/telemetry/policy.ts b/packages/studio/src/telemetry/policy.ts index 89f42ff71..9cf123dae 100644 --- a/packages/studio/src/telemetry/policy.ts +++ b/packages/studio/src/telemetry/policy.ts @@ -82,6 +82,17 @@ function isViteDevMode(): boolean { * (canary decisions do; the transports intentionally do not). */ export function browserTelemetryAllowed(): boolean { + try { + return allowed(); + } catch { + // Fail CLOSED. A storage read that throws must not enrol anyone, and must + // not propagate: callers include a post-commit catch block where a throw + // reports a committed edit as failed. + return false; + } +} + +function allowed(): boolean { return ( isApiKeyConfigured() && !isBuildTimeOptOut() &&