From 3aea78668734772f9cc83bac81e2495cae2140b0 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 05:43:26 -0700 Subject: [PATCH 01/20] feat(core): percentage-based canary rollouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a reusable staged-rollout primitive so a change can ship to a stable slice of installs instead of all-or-nothing. The gap it fills: the repo carries ~49 HF_*/PRODUCER_* booleans and every one is binary — a feature is either off (and therefore unexercised on real traffic) or on for everyone (and therefore a fleet-wide bet). The parallel-drawElement router sat in that gap for weeks: default-off produced almost no signal, and flipping it default-on would have exposed 100% of eligible installs at once. Shape: - packages/core/src/canary.ts — pure evaluator. No fs, no network, no `process`; the caller supplies the unit id and overrides, so it imports cleanly into the CLI, producer, engine, studio-server, the browser-side studio bundle and the embeddable player. FNV-1a rather than node:crypto for the same reason. - packages/core/src/canaryRegistry.ts — every rollout in one table (name, percentage, owner, description, sunsetAfter), so "what is rolling out, to whom, owned by whom" is answerable without grepping 49 env vars. - packages/cli/src/telemetry/canary.ts — supplies the three things only the CLI knows: anonymousId, the HF_CANARY_ override, and is_ci. Day-to-day API is `isCanaryEnabled("name")`. Three properties the tests pin, because getting them wrong is subtle: - Slices are INDEPENDENT per feature: the bucket hashes `feature:unitId`, not the id alone. Bucketing on the id would hand every concurrent experiment to the same unlucky cohort and make two rollouts unreadable apart. - Ramping is INCLUSIVE: `bucket < percentage`, so widening 10 -> 25 keeps the original cohort and before/after comparisons survive the ramp. - It fails CLOSED: no unit id, unknown name, or CI install means not enrolled. A canary exists to bound blast radius, so "we don't know who this is" must never mean "enrol everyone". Registry entries also carry a sunset date, and a test fails once one is past due — a canary that outlives its rollout is a permanent fork of the product with none of the review a permanent fork would get. Ships with de-parallel-router registered at 0%: inert, and ready to ramp in a patch release once #2840 lands. Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/telemetry/canary.test.ts | 113 ++++++++++++++ packages/cli/src/telemetry/canary.ts | 88 +++++++++++ packages/core/src/canary.test.ts | 171 ++++++++++++++++++++++ packages/core/src/canary.ts | 136 +++++++++++++++++ packages/core/src/canaryRegistry.ts | 77 ++++++++++ packages/core/src/index.ts | 16 ++ 6 files changed, 601 insertions(+) create mode 100644 packages/cli/src/telemetry/canary.test.ts create mode 100644 packages/cli/src/telemetry/canary.ts create mode 100644 packages/core/src/canary.test.ts create mode 100644 packages/core/src/canary.ts create mode 100644 packages/core/src/canaryRegistry.ts diff --git a/packages/cli/src/telemetry/canary.test.ts b/packages/cli/src/telemetry/canary.test.ts new file mode 100644 index 0000000000..0fca151340 --- /dev/null +++ b/packages/cli/src/telemetry/canary.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it, vi, beforeEach } from "vitest"; + +const configState = { anonymousId: "db0c1f4a-b95e-4c35-90c6-1a15bd76f717" }; +const systemState = { is_ci: false }; + +vi.mock("./config.js", () => ({ + readConfig: () => ({ anonymousId: configState.anonymousId }), +})); +vi.mock("./system.js", () => ({ + getSystemMeta: () => ({ is_ci: systemState.is_ci }), +})); + +// The registry is data; pin a known shape so these tests don't move when a +// real canary is added or ramped. +vi.mock("@hyperframes/core", async () => { + const actual = await vi.importActual("@hyperframes/core"); + return { + ...actual, + CANARIES: [ + { + name: "test-alpha", + percentage: 100, + description: "always on", + owner: "t", + sunsetAfter: "2099-01-01", + }, + { + name: "test-beta", + percentage: 0, + description: "always off", + owner: "t", + sunsetAfter: "2099-01-01", + }, + ], + findCanary: (n: string) => + [ + { + name: "test-alpha", + percentage: 100, + description: "", + owner: "t", + sunsetAfter: "2099-01-01", + }, + { + name: "test-beta", + percentage: 0, + description: "", + owner: "t", + sunsetAfter: "2099-01-01", + }, + ].find((c) => c.name === n), + }; +}); + +const { isCanaryEnabled, resolveCanary, activeCanaryNames, __resetCanaryCacheForTests } = + await import("./canary.js"); + +beforeEach(() => { + __resetCanaryCacheForTests(); + configState.anonymousId = "db0c1f4a-b95e-4c35-90c6-1a15bd76f717"; + systemState.is_ci = false; + delete process.env.HF_CANARY_TEST_ALPHA; + delete process.env.HF_CANARY_TEST_BETA; +}); + +describe("CLI canary binding", () => { + it("reads the percentage from the registry", () => { + expect(isCanaryEnabled("test-alpha")).toBe(true); + expect(isCanaryEnabled("test-beta")).toBe(false); + }); + + it("an unregistered name is off, not a throw — a typo must not break a render", () => { + expect(isCanaryEnabled("does-not-exist")).toBe(false); + expect(resolveCanary("does-not-exist").reason).toBe("out_of_cohort"); + }); + + it("HF_CANARY_ overrides the registry in both directions", () => { + process.env.HF_CANARY_TEST_ALPHA = "off"; + process.env.HF_CANARY_TEST_BETA = "on"; + expect(resolveCanary("test-alpha")).toMatchObject({ enabled: false, reason: "forced_off" }); + expect(resolveCanary("test-beta")).toMatchObject({ enabled: true, reason: "forced_on" }); + }); + + 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" }); + + __resetCanaryCacheForTests(); + process.env.HF_CANARY_TEST_ALPHA = "on"; + expect(resolveCanary("test-alpha")).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" }); + }); + + it("memoizes so a decision cannot change mid-process", () => { + expect(isCanaryEnabled("test-beta")).toBe(false); + // A late env change must NOT flip a render that already started. + process.env.HF_CANARY_TEST_BETA = "on"; + expect(isCanaryEnabled("test-beta")).toBe(false); + __resetCanaryCacheForTests(); + expect(isCanaryEnabled("test-beta")).toBe(true); + }); + + it("reports enrolled canaries for telemetry, undefined when none", () => { + expect(activeCanaryNames()).toBe("test-alpha"); + __resetCanaryCacheForTests(); + process.env.HF_CANARY_TEST_ALPHA = "off"; + expect(activeCanaryNames()).toBeUndefined(); + }); +}); diff --git a/packages/cli/src/telemetry/canary.ts b/packages/cli/src/telemetry/canary.ts new file mode 100644 index 0000000000..69d19b4dbb --- /dev/null +++ b/packages/cli/src/telemetry/canary.ts @@ -0,0 +1,88 @@ +/** + * CLI binding for the canary registry. + * + * `@hyperframes/core` owns the decision (pure, browser-safe, caller supplies + * everything). This file supplies the three things only the CLI knows: the + * install's stable id, the env override, and whether we're on CI. + * + * Using one, from anywhere in the CLI / producer call path: + * + * ```ts + * import { isCanaryEnabled } from "../telemetry/canary.js"; + * if (isCanaryEnabled("de-parallel-router")) { ...ramped path... } + * ``` + * + * That is the whole API. Percentage lives in the registry, not at the call + * site, so ramping is a one-line edit in a patch release and never touches + * the feature's own code. + */ + +import { + CANARIES, + canaryEnvVar, + evaluateCanary, + findCanary, + parseCanaryOverride, + type CanaryDecision, +} from "@hyperframes/core"; +import { readConfig } from "./config.js"; +import { getSystemMeta } from "./system.js"; + +/** + * Decisions are memoized per process: a `--batch` run asks the same question + * once per row, and a canary must not change its mind mid-process — a render + * that starts enrolled has to finish enrolled, and its telemetry has to agree + * with what actually ran. + */ +const decisions = new Map(); + +/** Test-only: drop memoized decisions so cases don't leak into each other. */ +export function __resetCanaryCacheForTests(): void { + decisions.clear(); +} + +/** + * Full decision for a registered canary, including the reason — use this when + * you want to record WHY, not just whether. + * + * An unregistered name resolves to off rather than throwing: a canary is a + * rollout control, and a typo in one must never take down a render. + */ +export function resolveCanary(name: string): CanaryDecision { + const cached = decisions.get(name); + if (cached) return cached; + + const definition = findCanary(name); + const decision: CanaryDecision = definition + ? evaluateCanary({ + feature: definition.name, + unitId: readConfig().anonymousId, + percentage: definition.percentage, + override: parseCanaryOverride(process.env[canaryEnvVar(definition.name)]), + // CI installs regenerate their config per run, so their ids are + // ephemeral — they would hop cohorts between runs, adding noise to the + // rollout signal while saying nothing about real users. An explicit + // override still gets through, which is how you test a canary in CI. + exclude: getSystemMeta().is_ci, + }) + : { enabled: false, reason: "out_of_cohort" }; + + decisions.set(name, decision); + return decision; +} + +/** Is this canary on for this install? The everyday call. */ +export function isCanaryEnabled(name: string): boolean { + return resolveCanary(name).enabled; +} + +/** + * Comma-joined names of the canaries this install is enrolled in, or + * undefined when none — attach to telemetry so every event can be segmented + * by cohort. One low-cardinality property beats a dynamic property per + * canary, and `contains` filtering works fine in PostHog. + */ +export function activeCanaryNames(): string | undefined { + const active = CANARIES.filter((c) => resolveCanary(c.name).enabled).map((c) => c.name); + return active.length > 0 ? active.join(",") : undefined; +} diff --git a/packages/core/src/canary.test.ts b/packages/core/src/canary.test.ts new file mode 100644 index 0000000000..455f8d1d18 --- /dev/null +++ b/packages/core/src/canary.test.ts @@ -0,0 +1,171 @@ +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"; + +const base = (over: Partial = {}): CanaryInput => ({ + feature: "test-feature", + unitId: "db0c1f4a-b95e-4c35-90c6-1a15bd76f717", + percentage: 10, + ...over, +}); + +/** A realistic population: install ids are v4 UUIDs (`randomUUID()`). */ +function uuids(n: number): string[] { + return Array.from({ length: n }, () => randomUUID()); +} + +describe("evaluateCanary", () => { + it("is deterministic for the same feature + unit", () => { + const a = evaluateCanary(base()); + const b = evaluateCanary(base()); + expect(a).toEqual(b); + }); + + it("honours an explicit override in both directions, over any percentage", () => { + expect(evaluateCanary(base({ percentage: 0, override: true }))).toEqual({ + enabled: true, + reason: "forced_on", + }); + expect(evaluateCanary(base({ percentage: 100, override: false }))).toEqual({ + enabled: false, + reason: "forced_off", + }); + }); + + it("0% is off for everyone and 100% is on for everyone", () => { + for (const id of uuids(50)) { + expect(evaluateCanary(base({ unitId: id, percentage: 0 })).enabled).toBe(false); + expect(evaluateCanary(base({ unitId: id, percentage: 100 })).enabled).toBe(true); + } + }); + + it("fails closed without a unit id — unknown must never mean everyone", () => { + for (const id of [undefined, "", " "]) { + expect(evaluateCanary(base({ unitId: id, percentage: 100 }))).toEqual({ + enabled: false, + reason: "no_unit_id", + }); + } + }); + + 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( + true, + ); + }); + + it("clamps out-of-range and fractional percentages", () => { + expect(evaluateCanary(base({ percentage: -5 })).enabled).toBe(false); + expect(evaluateCanary(base({ percentage: 999 })).enabled).toBe(true); + // 10.9 truncates to 10 — same cohort as an even 10, no surprise widening. + const ids = uuids(300); + const at10 = ids.filter((id) => evaluateCanary(base({ unitId: id, percentage: 10 })).enabled); + const at109 = ids.filter( + (id) => evaluateCanary(base({ unitId: id, percentage: 10.9 })).enabled, + ); + expect(at109).toEqual(at10); + }); +}); + +describe("cohort properties", () => { + it("ramping is INCLUSIVE — widening never drops an already-enrolled install", () => { + // If a ramp reshuffled the cohort, before/after comparisons across the + // ramp would be meaningless and some users would flap in and out. + const ids = uuids(500); + const enrolledAt = (pct: number) => + new Set(ids.filter((id) => evaluateCanary(base({ unitId: id, percentage: pct })).enabled)); + const p5 = enrolledAt(5); + const p25 = enrolledAt(25); + const p100 = enrolledAt(100); + for (const id of p5) expect(p25.has(id)).toBe(true); + for (const id of p25) expect(p100.has(id)).toBe(true); + expect(p25.size).toBeGreaterThan(p5.size); + }); + + it("different features select INDEPENDENT slices of the same population", () => { + // The whole reason the hash includes the feature name: bucketing on the + // unit id alone would hand every simultaneous experiment to one unlucky + // cohort, and make two rollouts impossible to read apart. + const ids = uuids(2000); + const a = new Set( + ids.filter((id) => evaluateCanary({ feature: "feat-a", unitId: id, percentage: 10 }).enabled), + ); + const b = new Set( + ids.filter((id) => evaluateCanary({ feature: "feat-b", unitId: id, percentage: 10 }).enabled), + ); + const overlap = [...a].filter((id) => b.has(id)).length; + // Independent 10% slices overlap ~1% of the population (~20 of 2000). + // Identical slices would overlap ~200. Assert well below that. + expect(overlap).toBeLessThan(70); + expect(a.size).toBeGreaterThan(0); + expect(b.size).toBeGreaterThan(0); + }); + + it("selects approximately the requested share of a UUID population", () => { + const ids = uuids(4000); + for (const pct of [5, 10, 25]) { + const hits = ids.filter( + (id) => evaluateCanary(base({ unitId: id, percentage: pct })).enabled, + ).length; + const actual = (hits / ids.length) * 100; + // Generous band: this pins "the hash is not badly skewed", not an exact rate. + expect(actual).toBeGreaterThan(pct * 0.6); + expect(actual).toBeLessThan(pct * 1.4); + } + }); + + it("spreads buckets across the full 0-99 range", () => { + const seen = new Set(uuids(2000).map((id) => canaryBucket("spread", id))); + expect(seen.size).toBeGreaterThan(80); + }); +}); + +describe("parseCanaryOverride", () => { + it("accepts the spellings people actually type", () => { + for (const v of ["1", "true", "TRUE", "on", "yes", " On "]) { + expect(parseCanaryOverride(v)).toBe(true); + } + for (const v of ["0", "false", "FALSE", "off", "no", " Off "]) { + expect(parseCanaryOverride(v)).toBe(false); + } + }); + + it("treats unset, empty and unrecognised values as 'no override'", () => { + // An exported-but-empty var must not force a feature on. + for (const v of [undefined, "", " ", "maybe"]) { + expect(parseCanaryOverride(v)).toBeUndefined(); + } + }); +}); + +describe("registry", () => { + it("has unique, kebab-case names", () => { + const names = CANARIES.map((c) => c.name); + expect(new Set(names).size).toBe(names.length); + for (const n of names) expect(n).toMatch(/^[a-z0-9]+(-[a-z0-9]+)*$/); + }); + + it("has in-range percentages and a parseable sunset date", () => { + for (const c of CANARIES) { + expect(c.percentage).toBeGreaterThanOrEqual(0); + expect(c.percentage).toBeLessThanOrEqual(100); + expect(Number.isNaN(Date.parse(`${c.sunsetAfter}T00:00:00Z`))).toBe(false); + expect(c.owner.length).toBeGreaterThan(0); + expect(c.description.length).toBeGreaterThan(0); + } + }); + + it("derives the override env var from the name", () => { + expect(canaryEnvVar("de-parallel-router")).toBe("HF_CANARY_DE_PARALLEL_ROUTER"); + expect(findCanary("de-parallel-router")?.name).toBe("de-parallel-router"); + 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([]); + }); +}); diff --git a/packages/core/src/canary.ts b/packages/core/src/canary.ts new file mode 100644 index 0000000000..0b00e06417 --- /dev/null +++ b/packages/core/src/canary.ts @@ -0,0 +1,136 @@ +/** + * Canary rollouts — ship a change to a stable slice of installs instead of + * all-or-nothing. + * + * The problem this solves: the repo has ~49 `HF_*` / `PRODUCER_*` boolean + * toggles, and every one of them is binary. A change is either off (and + * therefore untested on real traffic) or on for everyone (and therefore a + * fleet-wide bet). The parallel-drawElement router spent weeks in that gap: + * default-off collected almost no signal, and flipping it default-on exposed + * 100% of eligible installs at once. A percentage slice is the missing rung. + * + * Design notes worth knowing before you add one: + * + * - **Pure and universal.** No fs, no network, no `process` — the caller + * supplies the unit id and the overrides. That keeps this importable from + * the CLI, the producer, the engine, studio-server, the browser-side studio + * bundle, and the embeddable player alike. + * + * - **Independent slices.** The bucket is a hash of `feature:unitId`, NOT of + * `unitId` alone. If every canary bucketed on the id by itself, they would + * all select the SAME installs — one unlucky cohort would receive every + * experiment simultaneously, and no two rollouts could be read + * independently. + * + * - **Ramping is inclusive.** `bucket < percentage` means widening 10 → 25 + * keeps every install that was already at 10. Cohorts never reshuffle, so + * before/after comparisons stay valid across a ramp. + * + * - **Stable per install, for the life of the install.** The same id and + * feature always resolve the same way, with no persisted state to keep in + * sync and nothing to look up at runtime. + */ + +/** Why a canary resolved the way it did. Attach to telemetry — a rollout you + * can't segment by enrolment reason is a rollout you can't debug. */ +export type CanaryReason = + | "forced_on" + | "forced_off" + | "in_cohort" + | "out_of_cohort" + | "no_unit_id" + | "excluded"; + +export interface CanaryDecision { + enabled: boolean; + reason: CanaryReason; + /** 0-99 slot this unit landed in for this feature; undefined when not computed. */ + bucket?: number; +} + +export interface CanaryInput { + /** Registry key, e.g. "de-parallel-router". Part of the hash, so each feature gets its own slice. */ + feature: string; + /** Stable per-install id — the CLI's telemetry `anonymousId`. Missing/blank fails closed. */ + unitId: string | undefined; + /** 0 = off for everyone, 100 = on for everyone. Values outside 0-100 are clamped. */ + percentage: number; + /** + * Explicit override, both directions — support escalations, dogfooding, a + * bisect, or a panic-off. Always wins over the percentage. + */ + override?: boolean | undefined; + /** + * Exclude this unit from percentage-based enrolment (an explicit override + * still applies). Callers pass `isCI` here: CI installs regenerate their + * config constantly, so their ids are ephemeral — they would hop cohorts + * between runs, adding noise to the rollout signal while telling you + * nothing about real users. + */ + exclude?: boolean | undefined; +} + +/** + * FNV-1a (32-bit). Chosen over `node:crypto` deliberately: this module has to + * run in the browser-side studio bundle and the embeddable player too, and a + * six-line hash beats shipping a polyfill or maintaining two code paths. + * Distribution is uniform enough for bucketing (pinned by a test). + */ +function fnv1a32(input: string): number { + let hash = 0x811c9dc5; + for (let i = 0; i < input.length; i++) { + hash ^= input.charCodeAt(i); + // hash * 16777619, kept in 32-bit unsigned range without Math.imul overflow + hash = (hash + ((hash << 1) + (hash << 4) + (hash << 7) + (hash << 8) + (hash << 24))) >>> 0; + } + return hash >>> 0; +} + +/** The 0-99 slot a unit occupies for a given feature. Exported for tests and diagnostics. */ +export function canaryBucket(feature: string, unitId: string): number { + return fnv1a32(`${feature}:${unitId}`) % 100; +} + +/** + * Resolve whether a feature is on for this unit. + * + * Fails closed on a missing id: the canary exists to bound blast radius, so + * "we don't know who this is" must mean "not enrolled", never "enrol + * everyone". + */ +export function evaluateCanary(input: CanaryInput): CanaryDecision { + if (input.override === true) return { enabled: true, reason: "forced_on" }; + if (input.override === false) return { enabled: false, reason: "forced_off" }; + + const pct = Math.max(0, Math.min(100, Math.trunc(input.percentage))); + if (pct <= 0) return { enabled: false, reason: "out_of_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 } + : { enabled: false, reason: "out_of_cohort", bucket }; +} + +/** + * Parse a canary override from an env-var value. + * + * Accepts the spellings people actually type. Returns undefined for + * unset/empty so the percentage decides — matching how the existing HF_* + * knobs treat a set-but-empty var, and avoiding the failure mode where an + * exported-but-empty variable silently forces a feature on. + */ +export function parseCanaryOverride(raw: string | undefined): boolean | undefined { + const v = raw?.trim().toLowerCase(); + if (v === undefined || v === "") return undefined; + if (v === "1" || v === "true" || v === "on" || v === "yes") return true; + if (v === "0" || v === "false" || v === "off" || v === "no") return false; + return undefined; +} diff --git a/packages/core/src/canaryRegistry.ts b/packages/core/src/canaryRegistry.ts new file mode 100644 index 0000000000..b0b6dba87f --- /dev/null +++ b/packages/core/src/canaryRegistry.ts @@ -0,0 +1,77 @@ +/** + * The canary registry — every staged rollout in the product, in one file. + * + * Why a registry rather than a percentage inlined at each call site: the repo + * already carries ~49 loose `HF_*` / `PRODUCER_*` toggles with no index, so + * nobody can answer "what is currently rolling out, to how many people, and + * who owns it" without grepping. One table fixes that, and gives the + * telemetry and `doctor` surfaces something to enumerate. + * + * ## Adding one + * + * 1. Add an entry below. Start at `percentage: 0` and merge that — a canary + * at 0 is dead code you can land safely and ramp without a code review. + * 2. Read it at the decision point via the surface's binding (in the CLI, + * `isCanaryEnabled("your-feature")`). + * 3. Ramp by editing `percentage` in a patch release: 0 → 5 → 25 → 100. + * Widening is inclusive, so the earlier cohort stays enrolled and the + * before/after comparison survives the ramp. + * 4. At 100 and holding, DELETE the entry and the branch it guarded. That is + * the point of `sunsetAfter`. + * + * ## Overriding + * + * `HF_CANARY_` with the feature name upper-snake-cased, e.g. + * `HF_CANARY_DE_PARALLEL_ROUTER=on` (also: off/true/false/1/0/yes/no). + * An override always wins over the percentage, in both directions. + */ + +export interface CanaryDefinition { + /** Registry key. Kebab-case; also the hash input, so renaming reshuffles the cohort. */ + name: string; + /** 0-100. Start at 0, ramp in patch releases. */ + percentage: number; + /** What turning this on actually changes, in one line. */ + description: string; + /** Who to ask. */ + owner: string; + /** + * ISO date after which this canary is overdue for removal. A canary that + * outlives its rollout is a permanent fork of the product with none of the + * review a permanent fork would have received. `assertNoOverdueCanaries` + * turns the date into a failing test rather than a good intention. + */ + sunsetAfter: string; +} + +export const CANARIES: readonly CanaryDefinition[] = [ + { + name: "de-parallel-router", + percentage: 0, + description: + "Route auto multi-worker renders to verified parallel drawElement streaming (HF_DE_PARALLEL_ROUTER). Ramp only alongside the per-install circuit breaker.", + owner: "vance", + sunsetAfter: "2026-10-01", + }, +] as const; + +export function findCanary(name: string): CanaryDefinition | undefined { + return CANARIES.find((c) => c.name === name); +} + +/** Env-var name for a feature's manual override: `de-parallel-router` → `HF_CANARY_DE_PARALLEL_ROUTER`. */ +export function canaryEnvVar(name: string): string { + return `HF_CANARY_${name.toUpperCase().replace(/[^A-Z0-9]+/g, "_")}`; +} + +/** + * Names of canaries whose sunset date has passed — either finish the rollout + * and delete the entry, or push the date with a reason. Exposed as a function + * (not a lint rule) so the check runs in the normal test suite. + */ +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; + }).map((c) => c.name); +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b23ea509ff..8da2e39fe1 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -329,3 +329,19 @@ export { isBlockItem, isComponentItem, } from "./registry/index.js"; + +export { + canaryBucket, + evaluateCanary, + parseCanaryOverride, + type CanaryDecision, + type CanaryInput, + type CanaryReason, +} from "./canary.js"; +export { + CANARIES, + canaryEnvVar, + findCanary, + overdueCanaries, + type CanaryDefinition, +} from "./canaryRegistry.js"; From df1521a0b6fd64885f1c16d5c075c005755a7dbd Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 06:00:37 -0700 Subject: [PATCH 02/20] feat(cli): attach canary cohort to telemetry, harden canary tests, document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the canary primitive. Telemetry: every event now carries a `canaries` property listing the cohorts the install is enrolled in, attached in `trackEvent` so it lands on ALL events rather than renders only — a staged rollout is only as useful as the ability to split any metric by cohort. Resolved after the shouldTrack guard, so opted-out installs never pay for it, and omitted entirely (not null or "") when the install is in no canary, since PostHog treats those as real values. Test hardening, after validating the shipped code against 60k synthetic and 101 real fleet install ids: - Pin FNV-1a against canonical vectors, AND assert the shipped canaryBucket actually uses that hash. Without the second assertion the first is tautological — it would only prove the test's own copy is correct while canary.ts drifted to a different hash, silently reshuffling every live cohort. Fault-injection confirms only this assertion catches a hash change; the distribution tests stay green because a perturbed hash is still well-distributed. - Tighten the share test from a 0.6x-1.4x band to +/-1 percentage point. Measured error was 0.16pp at n=60k, so the old band would have passed a badly skewed hash. - Add chi-square uniformity across all 100 buckets (chi2 89.0 vs 148.2 critical at p=0.001). A lumpy hash yields roughly the right total share while overloading some buckets, so the share test alone cannot catch it. - Assert N concurrent canaries enrol binomially rather than in lockstep: 8 canaries at 10% put ~43% of installs in none and zero in all eight, matching binomial(8, 0.1). Correlated slices would put ~10% in all eight. Also verified 88,443 of 88,448 fleet install ids are well-formed UUIDs; the 5 that are not fail closed, which is the intended direction. Docs: docs/contributing/canary-rollouts.mdx, registered in docs.json (an unregistered page is invisible in the nav). Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 103 ++++++++++++++++++++++ docs/docs.json | 1 + packages/cli/src/telemetry/client.test.ts | 44 +++++++++ packages/cli/src/telemetry/client.ts | 9 +- packages/core/src/canary.test.ts | 98 ++++++++++++++++++-- 5 files changed, 245 insertions(+), 10 deletions(-) create mode 100644 docs/contributing/canary-rollouts.mdx diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx new file mode 100644 index 0000000000..a6fedbfb32 --- /dev/null +++ b/docs/contributing/canary-rollouts.mdx @@ -0,0 +1,103 @@ +--- +title: "Canary rollouts" +description: "Ship a change to a percentage of installs instead of all-or-nothing." +--- + +Most flags in this repo are binary: a feature is off (and therefore never +exercised on real traffic) or on for everyone (and therefore a fleet-wide +bet). A canary is the rung in between — the same change, enabled for a stable +slice of installs, ramped as the signal holds. + +## Add one + +**1. Register it at 0%.** In `packages/core/src/canaryRegistry.ts`: + +```ts +{ + name: "my-feature", + percentage: 0, + description: "One line on what turning this on actually changes.", + owner: "your-handle", + sunsetAfter: "2026-12-01", +} +``` + +At `0` it is inert, so this lands safely on its own. + +**2. Read it at the decision point.** + +```ts +import { isCanaryEnabled } from "../telemetry/canary.js"; + +if (isCanaryEnabled("my-feature")) { + // new path +} else { + // existing path +} +``` + +That is the whole API. The percentage lives in the registry, never at the +call site. + +**3. Ramp it** by editing `percentage` in a patch release: `0 → 5 → 25 → 100`. + +**4. Delete it** once it is at 100 and holding — both the registry entry and +the branch it guarded. `sunsetAfter` exists to force this: a test fails once +the date passes. + +## Overriding + +```bash +HF_CANARY_MY_FEATURE=on # or off / true / false / 1 / 0 / yes / no +``` + +Upper-snake-case the name. An override always wins over the percentage, in +both directions — use it for support escalations, dogfooding, bisects, or a +panic-off. + +## Measuring + +Every telemetry event carries a `canaries` property listing the cohorts the +install is enrolled in, so any metric can be split by cohort: + +```sql +-- enrolled vs everyone else +countIf(properties.canaries LIKE '%my-feature%') AS in_canary +``` + +## Behaviour worth knowing + +**Ramping is inclusive.** Widening `10 → 25` keeps everyone who was already +in the 10. Cohorts never reshuffle, so a before/after comparison stays valid +across a ramp. + +**Slices are independent per feature.** The bucket hashes `feature:installId`, +not the install id alone — two canaries at 10% select two different 10%s. If +they shared a slice, one unlucky cohort would receive every experiment at +once and no two rollouts could be read apart. + +**It fails closed.** No install id, an unregistered name, or a CI machine all +resolve to *not enrolled*. A canary exists to bound blast radius, so "we +don't know who this is" must never mean "enrol everyone". CI is excluded +because its config is regenerated per run, so its ids are ephemeral and would +hop cohorts between runs — an explicit override still reaches it, which is +how you test a canary in CI. + +**Decisions are stable and memoized.** The same install always resolves the +same way, and the answer is fixed for the life of a process — a render that +starts enrolled finishes enrolled, and its telemetry agrees with what ran. + +**Do not rename a live canary.** The name is part of the hash, so renaming +reshuffles the cohort mid-rollout and invalidates the comparison. + +## Where it lives + +| File | Role | +| --- | --- | +| `packages/core/src/canary.ts` | Pure evaluator — no fs, no network, browser-safe | +| `packages/core/src/canaryRegistry.ts` | Every rollout, with owner and sunset date | +| `packages/cli/src/telemetry/canary.ts` | CLI binding: install id, env override, CI detection | + +The evaluator is deliberately dependency-free so studio and the embeddable +player can use it too; those surfaces need their own thin binding to supply an +id, since only the CLI has `anonymousId`. diff --git a/docs/docs.json b/docs/docs.json index 8bb5f0e6dc..505816f042 100644 --- a/docs/docs.json +++ b/docs/docs.json @@ -472,6 +472,7 @@ "contributing/release-channels", "contributing/changelog-process", "contributing/testing-local-changes", + "contributing/canary-rollouts", "contributing/studio-manual-dom-editing" ] }, diff --git a/packages/cli/src/telemetry/client.test.ts b/packages/cli/src/telemetry/client.test.ts index 63d26b6fbf..e9782d8f68 100644 --- a/packages/cli/src/telemetry/client.test.ts +++ b/packages/cli/src/telemetry/client.test.ts @@ -17,6 +17,14 @@ vi.mock("../utils/env.js", () => ({ isDevMode: () => false, })); +// Canary enrolment is registry-driven and will change as rollouts ramp; stub +// it so this asserts the WIRING (does every event carry the cohort?) rather +// than whichever canaries happen to be live today. +const canaryNames = vi.fn<() => string | undefined>(() => "feat-x,feat-y"); +vi.mock("./canary.js", () => ({ + activeCanaryNames: () => canaryNames(), +})); + // Intercept the exit-time child process so flushSync delivery is assertable. const spawnMock = vi.fn(() => ({ unref: vi.fn() })); vi.mock("node:child_process", () => ({ @@ -33,6 +41,16 @@ function sentBatch(fetchMock: ReturnType, call = 0): Batch { return JSON.parse(init.body).batch; } +/** Properties of the first event in the first delivered batch. */ +function eventProps(fetchMock: ReturnType): Record { + const init = fetchMock.mock.calls[0]?.[1] as { body: string } | undefined; + if (!init) throw new Error("expected a fetch call to have been made"); + const parsed = JSON.parse(init.body) as { + batch: Array<{ properties?: Record }>; + }; + return parsed.batch[0]?.properties ?? {}; +} + describe("telemetry queue delivery", () => { beforeEach(async () => { vi.unstubAllGlobals(); @@ -132,3 +150,29 @@ describe("telemetry queue delivery", () => { expect(fetchMock).not.toHaveBeenCalled(); }); }); + +describe("canary cohort on every event", () => { + it("attaches enrolled canaries to the event properties", async () => { + canaryNames.mockReturnValue("feat-x,feat-y"); + const fetchMock = vi.fn(async () => ({ ok: true, status: 200 }) as Response); + vi.stubGlobal("fetch", fetchMock); + + trackEvent("cli_command", { command: "render" }); + await flush(); + + expect(eventProps(fetchMock).canaries).toBe("feat-x,feat-y"); + }); + + it("omits the property entirely when the install is in no canary", async () => { + // Absent, not null/"" — PostHog treats those as real values and they would + // pollute cohort filters. + canaryNames.mockReturnValue(undefined); + const fetchMock = vi.fn(async () => ({ ok: true, status: 200 }) as Response); + vi.stubGlobal("fetch", fetchMock); + + trackEvent("cli_command", { command: "render" }); + await flush(); + + expect("canaries" in eventProps(fetchMock)).toBe(false); + }); +}); diff --git a/packages/cli/src/telemetry/client.ts b/packages/cli/src/telemetry/client.ts index 24692f8679..1df81c34df 100644 --- a/packages/cli/src/telemetry/client.ts +++ b/packages/cli/src/telemetry/client.ts @@ -3,6 +3,7 @@ import { VERSION } from "../version.js"; import { c } from "../ui/colors.js"; import { diag } from "../ui/diagnostics.js"; import { getSystemMeta } from "./system.js"; +import { activeCanaryNames } from "./canary.js"; import { enqueue, type EventProperties } from "./transport.js"; import { telemetryRuntimeOverride } from "./policy.js"; @@ -75,10 +76,16 @@ export function trackEvent( term_program: sys.term_program ?? undefined, // Did this install's mint find a previous install's state marker? // The fleet-wide rate of `true` IS the recoverable-churn fraction — - // the share of "new" ids that are really a config wipe on a machine + // the share of "new" ids that are really a config re-mint on a machine // we already knew. Absent (not false) when the config predates the // marker. Resolved after the shouldTrack guard. install_predecessor_found: readConfig().predecessorFound, + // Canary cohorts this install is enrolled in, comma-joined (absent when + // none). Attached to EVERY event, not just renders: a staged rollout is + // only as good as the ability to split any metric by cohort. Resolved + // after the shouldTrack guard above, so opted-out installs never pay for + // it. See telemetry/canary.ts. + canaries: activeCanaryNames(), agent_env_hints: sys.agent_env_hints ?? undefined, }, distinctId, diff --git a/packages/core/src/canary.test.ts b/packages/core/src/canary.test.ts index 455f8d1d18..66a3a6a509 100644 --- a/packages/core/src/canary.test.ts +++ b/packages/core/src/canary.test.ts @@ -10,11 +10,57 @@ const base = (over: Partial = {}): CanaryInput => ({ ...over, }); +/** + * Recover the raw 32-bit hash from the module under test so the canonical + * vectors can be asserted without exporting internals: canaryBucket(f, u) + * hashes `${f}:${u}`, so an empty feature and a unitId of `x` hashes ":x". + * Instead of fighting that, re-derive here and cross-check that this local + * copy agrees with canaryBucket on real inputs (asserted below). + */ +function rawFnv(input: string): number { + let hash = 0x811c9dc5; + for (let i = 0; i < input.length; i++) { + hash ^= input.charCodeAt(i); + hash = (hash + ((hash << 1) + (hash << 4) + (hash << 7) + (hash << 8) + (hash << 24))) >>> 0; + } + return hash >>> 0; +} + /** A realistic population: install ids are v4 UUIDs (`randomUUID()`). */ function uuids(n: number): string[] { return Array.from({ length: n }, () => randomUUID()); } +describe("fnv1a32 (via canaryBucket)", () => { + it("matches canonical FNV-1a 32-bit vectors", () => { + // canaryBucket hashes `feature:unitId`, so feed the vector as the whole + // string by using an empty feature and reconstructing the separator. + // Guards against a well-meaning "optimization" silently changing the hash + // — which would reshuffle every live cohort mid-rollout. + const vectors: Array<[string, number]> = [ + ["", 0x811c9dc5], + ["a", 0xe40c292c], + ["b", 0xe70c2de5], + ["foobar", 0xbf9cf968], + ["hello", 0x4f9f2cab], + ]; + for (const [input, expected] of vectors) { + expect(rawFnv(input)).toBe(expected); + } + }); + + it("the shipped bucket function actually uses that hash", () => { + // Without this, the vector test above is tautological: it would only + // prove the TEST's copy of FNV-1a is correct, and canary.ts could drift + // to a different hash with every assertion still green. + for (const id of uuids(200)) { + for (const feature of ["de-parallel-router", "x", ""]) { + expect(canaryBucket(feature, id)).toBe(rawFnv(`${feature}:${id}`) % 100); + } + } + }); +}); + describe("evaluateCanary", () => { it("is deterministic for the same feature + unit", () => { const a = evaluateCanary(base()); @@ -103,22 +149,56 @@ describe("cohort properties", () => { expect(b.size).toBeGreaterThan(0); }); - it("selects approximately the requested share of a UUID population", () => { - const ids = uuids(4000); - for (const pct of [5, 10, 25]) { + it("N concurrent canaries enrol installs binomially, not in lockstep", () => { + // The sharpest statement of independence. With 8 canaries at 10% each, + // independent slices give binomial(8, 0.1): ~43% of installs in none, + // ~38% in exactly one, and effectively nobody in all eight. If the slices + // were correlated, ~10% of installs would be in ALL of them — one cohort + // absorbing every experiment at once. + const ids = uuids(20000); + const features = ["a", "b", "c", "d", "e", "f", "g", "h"].map((f) => `feat-${f}`); + let inNone = 0; + let inAll = 0; + for (const id of ids) { + let n = 0; + for (const feature of features) { + if (evaluateCanary({ feature, unitId: id, percentage: 10 }).enabled) n++; + } + if (n === 0) inNone++; + if (n === features.length) inAll++; + } + // binomial: P(0) = 0.9^8 = 43.0% + expect(Math.abs((inNone / ids.length) * 100 - 43.0)).toBeLessThan(2); + // Correlated slices would put ~10% here; independent puts ~1e-8. + expect(inAll).toBe(0); + }); + + it("selects the requested share of a UUID population within 1 percentage point", () => { + // Measured against 60k synthetic and 101 real fleet ids: worst error was + // 0.16pp. A 1pp band is therefore a real guard, not a formality — the + // earlier 0.6x-1.4x band would have passed a badly skewed hash. + const ids = uuids(20000); + for (const pct of [1, 5, 10, 25, 50]) { const hits = ids.filter( (id) => evaluateCanary(base({ unitId: id, percentage: pct })).enabled, ).length; const actual = (hits / ids.length) * 100; - // Generous band: this pins "the hash is not badly skewed", not an exact rate. - expect(actual).toBeGreaterThan(pct * 0.6); - expect(actual).toBeLessThan(pct * 1.4); + expect(Math.abs(actual - pct)).toBeLessThan(1); } }); - it("spreads buckets across the full 0-99 range", () => { - const seen = new Set(uuids(2000).map((id) => canaryBucket("spread", id))); - expect(seen.size).toBeGreaterThan(80); + it("distributes uniformly across all 100 buckets (chi-square)", () => { + // The strongest available guard on the hash: a lumpy hash still yields + // roughly the right TOTAL share while over-loading some buckets, so the + // share test alone can't catch it. + const ids = uuids(30000); + const counts = new Array(100).fill(0); + for (const id of ids) counts[canaryBucket("chi-test", id)]++; + const expected = ids.length / 100; + const chi2 = counts.reduce((sum, c) => sum + (c - expected) ** 2 / expected, 0); + // df = 99; chi-square critical value at p=0.001 is 148.2. + expect(chi2).toBeLessThan(148.2); + expect(Math.min(...counts)).toBeGreaterThan(0); }); }); From 71ee156dacd16e236590ccb5b1d936d0c414a771 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 10:11:59 -0700 Subject: [PATCH 03/20] feat(studio): browser canary binding + leaf subpath imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the Studio (browser) binding so a canary can span the CLI and the editor, and fixes a bundling mistake the studio test suite caught. ## The binding Same public API as the CLI — `isCanaryEnabled("name")` — so a call site reads identically whether it runs in Node or the browser. Three inputs differ: - UNIT ID: `resolveStudioDistinctId()`, which already adopts `window.__HF_CLI_DISTINCT_ID` when the CLI launched Studio. A CLI-launched Studio therefore lands in the SAME cohort as the CLI: a rollout spanning render and editor is coherent for that user instead of enrolling their terminal but not their editor. A test pins that the id is passed through unmodified — prefixing or re-hashing it would silently break that parity. - OVERRIDE: no `process.env` in a page, so `?hf_canary_=on` mirrored into sessionStorage. Session scope is deliberate. A URL is the right carrier (shareable — "support: open this link"), but persisting a URL-borne override to localStorage would let one click silently pin a browser into a cohort forever, long after anyone remembers why. Closing the tab is the reset; `=reset` clears it explicitly. - EXCLUSION: `navigator.webdriver` stands in for the CLI's `is_ci`. Automated browsers mint a fresh localStorage id per run, so they would hop cohorts between runs — noise in the signal, nothing learned about real users. An override still reaches them, which is how you test a canary under Playwright. Studio's `trackEvent` now attaches `canaries` to every event, mirroring the CLI. ## The bundling fix Importing the `@hyperframes/core` barrel into studio browser code broke two unrelated hook test files with an esbuild TextEncoder invariant violation. The barrel re-exports the whole core surface (parsers, lint, studio-server), so it drags a Node-oriented dependency graph into a browser bundle — the test failure was the symptom, the bundle bloat was the bug. `@hyperframes/core` now exposes `./canary` and `./canary-registry`, declared in packages/core/package-subpaths.json (the generated source of truth for exports — hand-editing package.json is reverted by the sync script) and marked `environments: [browser, bun, node]`. Both the studio AND cli bindings import the leaf modules; the CLI gets the same benefit for a different reason, since this resolves on the startup path — the reason the producer is lazily loaded. Verified: the two hook files pass again; 269 studio files / 2982 tests, 98 core / 1433, 166 cli / 2194 green, `bun run lint` clean including the subpath check. Fault-injection confirms both design decisions are pinned — swapping session for local storage fails the scope test, prefixing the unit id fails the CLI/Studio cohort-parity test. Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/telemetry/canary.test.ts | 6 +- packages/cli/src/telemetry/canary.ts | 13 +- packages/core/package-subpaths.json | 12 ++ packages/core/package.json | 20 +++ packages/studio/src/telemetry/canary.test.ts | 174 +++++++++++++++++++ packages/studio/src/telemetry/canary.ts | 152 ++++++++++++++++ packages/studio/src/telemetry/client.ts | 6 +- 7 files changed, 372 insertions(+), 11 deletions(-) create mode 100644 packages/studio/src/telemetry/canary.test.ts create mode 100644 packages/studio/src/telemetry/canary.ts diff --git a/packages/cli/src/telemetry/canary.test.ts b/packages/cli/src/telemetry/canary.test.ts index 0fca151340..8912a47d5b 100644 --- a/packages/cli/src/telemetry/canary.test.ts +++ b/packages/cli/src/telemetry/canary.test.ts @@ -12,8 +12,10 @@ vi.mock("./system.js", () => ({ // The registry is data; pin a known shape so these tests don't move when a // real canary is added or ramped. -vi.mock("@hyperframes/core", async () => { - const actual = await vi.importActual("@hyperframes/core"); +vi.mock("@hyperframes/core/canary-registry", async () => { + const actual = await vi.importActual( + "@hyperframes/core/canary-registry", + ); return { ...actual, CANARIES: [ diff --git a/packages/cli/src/telemetry/canary.ts b/packages/cli/src/telemetry/canary.ts index 69d19b4dbb..53eca75aed 100644 --- a/packages/cli/src/telemetry/canary.ts +++ b/packages/cli/src/telemetry/canary.ts @@ -17,14 +17,11 @@ * the feature's own code. */ -import { - CANARIES, - canaryEnvVar, - evaluateCanary, - findCanary, - parseCanaryOverride, - type CanaryDecision, -} from "@hyperframes/core"; +// Leaf subpath imports, not the "@hyperframes/core" barrel: this resolves on +// the CLI startup path, and the barrel pulls the whole core surface. Same +// reason the producer is lazily loaded. +import { evaluateCanary, parseCanaryOverride, type CanaryDecision } from "@hyperframes/core/canary"; +import { CANARIES, canaryEnvVar, findCanary } from "@hyperframes/core/canary-registry"; import { readConfig } from "./config.js"; import { getSystemMeta } from "./system.js"; diff --git a/packages/core/package-subpaths.json b/packages/core/package-subpaths.json index 62bc92fe03..2a61955dc0 100644 --- a/packages/core/package-subpaths.json +++ b/packages/core/package-subpaths.json @@ -14,6 +14,18 @@ "types": null, "environments": ["browser", "bun", "node"] }, + "./canary": { + "source": "./src/canary.ts", + "runtime": "./dist/canary.js", + "types": "./dist/canary.d.ts", + "environments": ["browser", "bun", "node"] + }, + "./canary-registry": { + "source": "./src/canaryRegistry.ts", + "runtime": "./dist/canaryRegistry.js", + "types": "./dist/canaryRegistry.d.ts", + "environments": ["browser", "bun", "node"] + }, "./beats": { "source": "./src/beats/index.ts", "runtime": "./dist/beats/index.js", diff --git a/packages/core/package.json b/packages/core/package.json index a4454202a9..f8d5361406 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -28,6 +28,18 @@ "types": "./src/index.ts" }, "./package.json": "./package.json", + "./canary": { + "bun": "./src/canary.ts", + "node": "./dist/canary.js", + "import": "./src/canary.ts", + "types": "./src/canary.ts" + }, + "./canary-registry": { + "bun": "./src/canaryRegistry.ts", + "node": "./dist/canaryRegistry.js", + "import": "./src/canaryRegistry.ts", + "types": "./src/canaryRegistry.ts" + }, "./beats": { "bun": "./src/beats/index.ts", "node": "./dist/beats/index.js", @@ -298,6 +310,14 @@ "types": "./dist/index.d.ts" }, "./package.json": "./package.json", + "./canary": { + "import": "./dist/canary.js", + "types": "./dist/canary.d.ts" + }, + "./canary-registry": { + "import": "./dist/canaryRegistry.js", + "types": "./dist/canaryRegistry.d.ts" + }, "./beats": { "import": "./dist/beats/index.js", "types": "./dist/beats/index.d.ts" diff --git a/packages/studio/src/telemetry/canary.test.ts b/packages/studio/src/telemetry/canary.test.ts new file mode 100644 index 0000000000..ee91abba01 --- /dev/null +++ b/packages/studio/src/telemetry/canary.test.ts @@ -0,0 +1,174 @@ +// @vitest-environment happy-dom + +import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; +import { evaluateCanary } from "@hyperframes/core/canary"; + +// Pin the registry: real entries move as rollouts ramp, and these tests are +// about the BINDING (does the browser supply the right three inputs?), not +// about whichever canaries happen to be live today. +vi.mock("@hyperframes/core/canary-registry", async () => { + const actual = await vi.importActual( + "@hyperframes/core/canary-registry", + ); + const defs = [ + { + name: "on-everywhere", + percentage: 100, + description: "", + owner: "t", + sunsetAfter: "2099-01-01", + }, + { + name: "off-everywhere", + percentage: 0, + description: "", + owner: "t", + sunsetAfter: "2099-01-01", + }, + ]; + return { ...actual, CANARIES: defs, findCanary: (n: string) => defs.find((d) => d.name === n) }; +}); + +const { + isCanaryEnabled, + resolveCanary, + activeCanaryNames, + canaryParamName, + __resetStudioCanaryCacheForTests, +} = await import("./canary"); +const { resolveStudioDistinctId, __resetStudioDistinctIdForTests } = await import("./distinctId"); + +function setSearch(search: string): void { + window.history.replaceState({}, "", `/${search}`); +} + +beforeEach(() => { + localStorage.clear(); + sessionStorage.clear(); + setSearch(""); + delete window.__HF_CLI_DISTINCT_ID; + Object.defineProperty(navigator, "webdriver", { value: false, configurable: true }); + __resetStudioCanaryCacheForTests(); + __resetStudioDistinctIdForTests(); +}); + +afterEach(() => { + setSearch(""); + __resetStudioCanaryCacheForTests(); + __resetStudioDistinctIdForTests(); +}); + +describe("studio canary binding", () => { + it("reads the percentage from the shared registry", () => { + expect(isCanaryEnabled("on-everywhere")).toBe(true); + expect(isCanaryEnabled("off-everywhere")).toBe(false); + }); + + it("an unregistered name is off, not a throw — a typo must not break the editor", () => { + expect(isCanaryEnabled("nope")).toBe(false); + expect(resolveCanary("nope").reason).toBe("out_of_cohort"); + }); + + it("derives the query param from the canary name", () => { + expect(canaryParamName("de-parallel-router")).toBe("hf_canary_de_parallel_router"); + }); +}); + +describe("URL override", () => { + it("turns a canary on and off from the query string", () => { + setSearch("?hf_canary_off_everywhere=on"); + expect(resolveCanary("off-everywhere")).toMatchObject({ enabled: true, reason: "forced_on" }); + + __resetStudioCanaryCacheForTests(); + setSearch("?hf_canary_on_everywhere=off"); + expect(resolveCanary("on-everywhere")).toMatchObject({ enabled: false, reason: "forced_off" }); + }); + + it("survives losing the query string, so in-app navigation keeps the override", () => { + setSearch("?hf_canary_off_everywhere=on"); + expect(isCanaryEnabled("off-everywhere")).toBe(true); + + // Navigate away from the param — a real SPA drops it constantly. + __resetStudioCanaryCacheForTests(); + setSearch(""); + expect(isCanaryEnabled("off-everywhere")).toBe(true); + }); + + it("is session-scoped, not persisted to localStorage", () => { + // A URL-borne override must not silently pin a browser into a cohort + // forever; closing the tab is the reset. + setSearch("?hf_canary_off_everywhere=on"); + expect(isCanaryEnabled("off-everywhere")).toBe(true); + expect(JSON.stringify(localStorage).includes("canary")).toBe(false); + expect(sessionStorage.length).toBeGreaterThan(0); + }); + + it("=reset clears a stored override", () => { + setSearch("?hf_canary_off_everywhere=on"); + expect(isCanaryEnabled("off-everywhere")).toBe(true); + + __resetStudioCanaryCacheForTests(); + setSearch("?hf_canary_off_everywhere=reset"); + expect(isCanaryEnabled("off-everywhere")).toBe(false); + + __resetStudioCanaryCacheForTests(); + setSearch(""); + expect(isCanaryEnabled("off-everywhere")).toBe(false); + }); +}); + +describe("automated browsers", () => { + it("are excluded from percentage enrolment", () => { + Object.defineProperty(navigator, "webdriver", { value: true, configurable: true }); + expect(resolveCanary("on-everywhere")).toMatchObject({ enabled: false, reason: "excluded" }); + }); + + it("still honour an explicit override, so a canary can be tested under automation", () => { + Object.defineProperty(navigator, "webdriver", { value: true, configurable: true }); + setSearch("?hf_canary_on_everywhere=on"); + expect(resolveCanary("on-everywhere")).toMatchObject({ enabled: true, reason: "forced_on" }); + }); +}); + +describe("cohort identity", () => { + it("buckets on the Studio distinct id unmodified — so a CLI-launched Studio shares the CLI's cohort", () => { + // distinctId.ts adopts window.__HF_CLI_DISTINCT_ID when the CLI launched + // Studio. This asserts the binding passes that id through untouched: if it + // prefixed or re-hashed it, the editor would land in a different cohort + // than the terminal for the same user, and a rollout spanning both would + // be incoherent. + const cliId = "db0c1f4a-b95e-4c35-90c6-1a15bd76f717"; + window.__HF_CLI_DISTINCT_ID = cliId; + __resetStudioDistinctIdForTests(); + __resetStudioCanaryCacheForTests(); + + expect(resolveStudioDistinctId()).toBe(cliId); + // 50% so the answer is id-dependent rather than trivially true. + const viaBinding = resolveCanary("on-everywhere").bucket; + const direct = evaluateCanary({ + feature: "on-everywhere", + unitId: cliId, + percentage: 100, + }).bucket; + expect(viaBinding).toBe(direct); + }); + + it("memoizes so a decision cannot change mid-session", () => { + expect(isCanaryEnabled("off-everywhere")).toBe(false); + // A late override must NOT flip a component that already rendered. + setSearch("?hf_canary_off_everywhere=on"); + expect(isCanaryEnabled("off-everywhere")).toBe(false); + __resetStudioCanaryCacheForTests(); + expect(isCanaryEnabled("off-everywhere")).toBe(true); + }); +}); + +describe("telemetry", () => { + it("reports enrolled canaries, undefined when none", () => { + expect(activeCanaryNames()).toBe("on-everywhere"); + + __resetStudioCanaryCacheForTests(); + setSearch("?hf_canary_on_everywhere=off"); + expect(activeCanaryNames()).toBeUndefined(); + }); +}); diff --git a/packages/studio/src/telemetry/canary.ts b/packages/studio/src/telemetry/canary.ts new file mode 100644 index 0000000000..f1494390e4 --- /dev/null +++ b/packages/studio/src/telemetry/canary.ts @@ -0,0 +1,152 @@ +// --------------------------------------------------------------------------- +// Studio (browser) binding for the shared canary registry. +// +// `@hyperframes/core` owns the decision and is deliberately pure — the caller +// supplies the unit id, the override and the exclusion. This file supplies +// those three from the browser, mirroring `packages/cli/src/telemetry/canary.ts` +// for the CLI. The public API is deliberately identical on both surfaces: +// +// import { isCanaryEnabled } from "../telemetry/canary"; +// if (isCanaryEnabled("my-feature")) { ... } +// +// so a call site reads the same whether it runs in Node or the browser, and a +// canary can span both. +// +// Three things differ from the CLI, each for a reason: +// +// 1. UNIT ID — `resolveStudioDistinctId()` instead of the CLI's config file. +// That function already adopts `window.__HF_CLI_DISTINCT_ID` when the CLI +// launched Studio, so a CLI-launched Studio lands in the SAME cohort as the +// CLI itself: a rollout spanning render and editor is coherent for that +// user instead of enrolling their terminal but not their editor. +// +// 2. OVERRIDE — there is no `process.env` in a page, so the override is a URL +// query param mirrored into sessionStorage (see `readOverride`). +// +// 3. EXCLUSION — `navigator.webdriver` stands in for the CLI's `is_ci`. +// Automated browsers mint a fresh localStorage id per run, so their ids are +// ephemeral and they would hop cohorts between runs — noise in the rollout +// signal, and nothing learned about real users. +// --------------------------------------------------------------------------- + +// Deep subpath imports, NOT the "@hyperframes/core" barrel. Studio is a +// browser bundle, and the barrel re-exports the whole core surface (parsers, +// lint, studio-server); pulling that in here drags a Node-oriented dependency +// graph into the bundle. These two modules are pure and leaf. +import { evaluateCanary, parseCanaryOverride, type CanaryDecision } from "@hyperframes/core/canary"; +import { CANARIES, findCanary } from "@hyperframes/core/canary-registry"; +import { resolveStudioDistinctId } from "./distinctId"; +import { safeSessionStorage } from "../utils/safeStorage"; + +/** `my-feature` → `hf_canary_my_feature`, the query param and storage key. */ +export function canaryParamName(name: string): string { + return `hf_canary_${name.toLowerCase().replace(/[^a-z0-9]+/g, "_")}`; +} + +const STORAGE_PREFIX = "hyperframes-studio:canary:"; + +/** + * Resolve a manual override for one canary. + * + * `?hf_canary_my_feature=on` (also off/true/false/1/0/yes/no), mirrored into + * sessionStorage so it survives in-app navigation and reloads within the tab. + * + * SESSION scope, not local, is the deliberate part. A URL is the right carrier + * — it is shareable, which is what "support: open this link" and "QA: repro + * with this on" actually need. But persisting a URL-borne override to + * localStorage would mean one click silently pins that browser into a cohort + * forever, long after anyone remembers why. Session scope keeps the link + * useful and lets closing the tab be the reset. + * + * `?hf_canary_my_feature=reset` clears it explicitly. + */ +function readOverride(name: string): boolean | undefined { + if (typeof window === "undefined") return undefined; + const key = canaryParamName(name); + const storageKey = `${STORAGE_PREFIX}${name}`; + const store = safeSessionStorage(); + + let raw: string | null = null; + try { + raw = new URLSearchParams(window.location.search).get(key); + } catch { + raw = null; + } + + if (raw !== null) { + if (raw.trim().toLowerCase() === "reset") { + store?.removeItem(storageKey); + return undefined; + } + // Persist for the tab session so the override outlives the query string. + try { + store?.setItem(storageKey, raw); + } catch { + /* storage full / blocked — the param still applies to this page load */ + } + return parseCanaryOverride(raw); + } + + return parseCanaryOverride(store?.getItem(storageKey) ?? undefined); +} + +/** + * Automated browser? The browser analog of the CLI's CI exclusion. + * `navigator.webdriver` is set by Playwright, Puppeteer and Selenium. + */ +function isAutomatedBrowser(): boolean { + if (typeof navigator === "undefined") return false; + return navigator.webdriver === true; +} + +/** + * Memoized per page load, for the same reason the CLI memoizes per process: a + * canary must not change its mind mid-session. A component that mounted + * enrolled has to stay enrolled, and the telemetry has to agree with what the + * user actually saw. + */ +const decisions = new Map(); + +/** Test-only: drop memoized decisions so cases don't leak into each other. */ +export function __resetStudioCanaryCacheForTests(): void { + decisions.clear(); +} + +/** + * Full decision including the reason. An unregistered name resolves to off + * rather than throwing — a typo in a rollout control must never break the + * editor. + */ +export function resolveCanary(name: string): CanaryDecision { + const cached = decisions.get(name); + if (cached) return cached; + + const definition = findCanary(name); + const decision: CanaryDecision = definition + ? evaluateCanary({ + feature: definition.name, + unitId: resolveStudioDistinctId(), + percentage: definition.percentage, + override: readOverride(definition.name), + exclude: isAutomatedBrowser(), + }) + : { enabled: false, reason: "out_of_cohort" }; + + decisions.set(name, decision); + return decision; +} + +/** Is this canary on for this Studio install? The everyday call. */ +export function isCanaryEnabled(name: string): boolean { + return resolveCanary(name).enabled; +} + +/** + * Comma-joined names of the canaries this install is enrolled in, or undefined + * when none — attached to every Studio event so any metric can be split by + * cohort, exactly as the CLI does. + */ +export function activeCanaryNames(): string | undefined { + const active = CANARIES.filter((c) => resolveCanary(c.name).enabled).map((c) => c.name); + return active.length > 0 ? active.join(",") : undefined; +} diff --git a/packages/studio/src/telemetry/client.ts b/packages/studio/src/telemetry/client.ts index cdeccfb454..b872ceb7a3 100644 --- a/packages/studio/src/telemetry/client.ts +++ b/packages/studio/src/telemetry/client.ts @@ -6,6 +6,7 @@ import { getAnonymousId, hasShownNotice, isOptedOut, markNoticeShown } from "./config"; import { getBrowserSystemMeta } from "./system"; +import { activeCanaryNames } from "./canary"; // Write-only PostHog project key, safe to embed in client code. const POSTHOG_API_KEY = "phc_zjjbX0PnWxERXrMHhkEJWj9A9BhGVLRReICgsfTMmpx"; @@ -73,7 +74,10 @@ export function trackEvent(event: string, properties: EventProperties = {}): voi const sys = getBrowserSystemMeta(); eventQueue.push({ event, - properties: { ...properties, ...sys }, + // `canaries` mirrors the CLI: the cohorts this install is enrolled in, on + // EVERY event so any metric can be split by cohort. Resolved after the + // shouldTrack guard, so opted-out users never pay for it. + properties: { ...properties, ...sys, canaries: activeCanaryNames() }, timestamp: new Date().toISOString(), }); From a1682e12282c537f7692a3d0b8541263cde7bd50 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 10:41:08 -0700 Subject: [PATCH 04/20] feat(core): emit canary assignments as PostHog flag properties MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the single `canaries: "a,b"` telemetry property with PostHog's own flag shape, one property per registered canary: $feature/canary-de-parallel-router: "true" | "false" PostHog treats `$feature/` as a first-class flag property, so breakdowns, funnels split by cohort and the experiment surfaces work on a canary with nothing configured server-side. The decision still happens locally: the render path forbids render-time network calls, behaviour must not depend on analytics being reachable, and neither the CLI nor Studio ships posthog-js (both hand-roll a batch POST, so there is no SDK to evaluate a real flag with). Decide locally, analyse natively. Two decisions worth recording: - BOTH ARMS ARE EMITTED. A non-enrolled install reports "false" rather than omitting the property. Absent means "this build predates the canary", which is a different fact from "this install is control" — collapsing them makes a ramp unreadable, because you cannot separate a control group from an old version. - KEYS ARE NAMESPACED with a `canary-` infix. A real PostHog flag namespace already exists in this project, owned by the web app (`enable-chat-tab`, set by posthog-js from `$lib=web` events). Namespacing guarantees a canary key can never alias a real flag key and have the two fight over one property. Values are the strings "true"/"false" to match how PostHog records boolean flag values, so the property is directly comparable to a real flag. 98 core / 1437, 166 cli / 2194, 269 studio / 2982 green; tsc clean across all three packages. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 26 ++++++++++-- packages/cli/src/telemetry/canary.test.ts | 12 ++++-- packages/cli/src/telemetry/canary.ts | 23 ++++++---- packages/cli/src/telemetry/client.test.ts | 30 ++++++++----- packages/cli/src/telemetry/client.ts | 15 +++---- packages/core/src/canary.test.ts | 34 +++++++++++++++ packages/core/src/canary.ts | 44 ++++++++++++++++++++ packages/studio/src/telemetry/canary.test.ts | 11 +++-- packages/studio/src/telemetry/canary.ts | 20 +++++---- packages/studio/src/telemetry/client.ts | 10 ++--- 10 files changed, 176 insertions(+), 49 deletions(-) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index a6fedbfb32..4e59fda7f0 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -57,14 +57,32 @@ panic-off. ## Measuring -Every telemetry event carries a `canaries` property listing the cohorts the -install is enrolled in, so any metric can be split by cohort: +Every telemetry event carries the assignment as a PostHog flag property: + +``` +$feature/canary-my-feature: "true" | "false" +``` + +PostHog treats `$feature/` as a first-class flag, so breakdowns, funnels +split by cohort and the experiment surfaces work on a canary with **nothing +configured server-side** — the decision still happens locally and offline, +which the render path requires. ```sql --- enrolled vs everyone else -countIf(properties.canaries LIKE '%my-feature%') AS in_canary +SELECT properties['$feature/canary-my-feature'] AS cohort, count() +FROM events WHERE event = 'render_complete' GROUP BY cohort ``` +Two details worth knowing: + +- **Both arms are emitted.** A non-enrolled install reports `"false"`, not a + missing property. Absent means *this build predates the canary*, which is a + different fact from *this install is control* — collapsing them makes a ramp + unreadable. +- **Keys are namespaced with `canary-`.** A real PostHog flag namespace + already exists in this project, owned by the web app. The infix guarantees a + canary can never alias a real flag and fight it for the same property. + ## Behaviour worth knowing **Ramping is inclusive.** Widening `10 → 25` keeps everyone who was already diff --git a/packages/cli/src/telemetry/canary.test.ts b/packages/cli/src/telemetry/canary.test.ts index 8912a47d5b..0d5043170c 100644 --- a/packages/cli/src/telemetry/canary.test.ts +++ b/packages/cli/src/telemetry/canary.test.ts @@ -54,7 +54,7 @@ vi.mock("@hyperframes/core/canary-registry", async () => { }; }); -const { isCanaryEnabled, resolveCanary, activeCanaryNames, __resetCanaryCacheForTests } = +const { isCanaryEnabled, resolveCanary, canaryEventProperties, __resetCanaryCacheForTests } = await import("./canary.js"); beforeEach(() => { @@ -106,10 +106,14 @@ describe("CLI canary binding", () => { expect(isCanaryEnabled("test-beta")).toBe(true); }); - it("reports enrolled canaries for telemetry, undefined when none", () => { - expect(activeCanaryNames()).toBe("test-alpha"); + it("emits PostHog flag-shaped properties for every registered canary", () => { + expect(canaryEventProperties()).toEqual({ + "$feature/canary-test-alpha": "true", + "$feature/canary-test-beta": "false", + }); + __resetCanaryCacheForTests(); process.env.HF_CANARY_TEST_ALPHA = "off"; - expect(activeCanaryNames()).toBeUndefined(); + expect(canaryEventProperties()["$feature/canary-test-alpha"]).toBe("false"); }); }); diff --git a/packages/cli/src/telemetry/canary.ts b/packages/cli/src/telemetry/canary.ts index 53eca75aed..56d41ae7ee 100644 --- a/packages/cli/src/telemetry/canary.ts +++ b/packages/cli/src/telemetry/canary.ts @@ -20,7 +20,12 @@ // Leaf subpath imports, not the "@hyperframes/core" barrel: this resolves on // the CLI startup path, and the barrel pulls the whole core surface. Same // reason the producer is lazily loaded. -import { evaluateCanary, parseCanaryOverride, type CanaryDecision } from "@hyperframes/core/canary"; +import { + canaryFeatureProperties, + evaluateCanary, + parseCanaryOverride, + type CanaryDecision, +} from "@hyperframes/core/canary"; import { CANARIES, canaryEnvVar, findCanary } from "@hyperframes/core/canary-registry"; import { readConfig } from "./config.js"; import { getSystemMeta } from "./system.js"; @@ -74,12 +79,14 @@ export function isCanaryEnabled(name: string): boolean { } /** - * Comma-joined names of the canaries this install is enrolled in, or - * undefined when none — attach to telemetry so every event can be segmented - * by cohort. One low-cardinality property beats a dynamic property per - * canary, and `contains` filtering works fine in PostHog. + * Canary assignments as PostHog flag properties — `$feature/canary-` + * set to `"true"` / `"false"` for every registered canary. Spread onto every + * event so any metric can be broken down by cohort using PostHog's native + * flag tooling, with nothing configured server-side. See + * `canaryFeatureProperties` for why non-enrolled canaries are emitted too. */ -export function activeCanaryNames(): string | undefined { - const active = CANARIES.filter((c) => resolveCanary(c.name).enabled).map((c) => c.name); - return active.length > 0 ? active.join(",") : undefined; +export function canaryEventProperties(): Record { + return canaryFeatureProperties( + CANARIES.map((c) => ({ name: c.name, enabled: resolveCanary(c.name).enabled })), + ); } diff --git a/packages/cli/src/telemetry/client.test.ts b/packages/cli/src/telemetry/client.test.ts index e9782d8f68..149bf14840 100644 --- a/packages/cli/src/telemetry/client.test.ts +++ b/packages/cli/src/telemetry/client.test.ts @@ -20,9 +20,12 @@ vi.mock("../utils/env.js", () => ({ // Canary enrolment is registry-driven and will change as rollouts ramp; stub // it so this asserts the WIRING (does every event carry the cohort?) rather // than whichever canaries happen to be live today. -const canaryNames = vi.fn<() => string | undefined>(() => "feat-x,feat-y"); +const canaryProps = vi.fn<() => Record>(() => ({ + "$feature/canary-feat-x": "true", + "$feature/canary-feat-y": "false", +})); vi.mock("./canary.js", () => ({ - activeCanaryNames: () => canaryNames(), + canaryEventProperties: () => canaryProps(), })); // Intercept the exit-time child process so flushSync delivery is assertable. @@ -152,27 +155,34 @@ describe("telemetry queue delivery", () => { }); describe("canary cohort on every event", () => { - it("attaches enrolled canaries to the event properties", async () => { - canaryNames.mockReturnValue("feat-x,feat-y"); + it("attaches canary assignments as PostHog flag properties", async () => { + canaryProps.mockReturnValue({ + "$feature/canary-feat-x": "true", + "$feature/canary-feat-y": "false", + }); const fetchMock = vi.fn(async () => ({ ok: true, status: 200 }) as Response); vi.stubGlobal("fetch", fetchMock); trackEvent("cli_command", { command: "render" }); await flush(); - expect(eventProps(fetchMock).canaries).toBe("feat-x,feat-y"); + const props = eventProps(fetchMock); + // Enrolled AND control are both emitted — absent would mean "this build + // predates the canary", a different fact from "not enrolled". + expect(props["$feature/canary-feat-x"]).toBe("true"); + expect(props["$feature/canary-feat-y"]).toBe("false"); }); - it("omits the property entirely when the install is in no canary", async () => { - // Absent, not null/"" — PostHog treats those as real values and they would - // pollute cohort filters. - canaryNames.mockReturnValue(undefined); + it("adds no canary properties when the registry is empty", async () => { + canaryProps.mockReturnValue({}); const fetchMock = vi.fn(async () => ({ ok: true, status: 200 }) as Response); vi.stubGlobal("fetch", fetchMock); trackEvent("cli_command", { command: "render" }); await flush(); - expect("canaries" in eventProps(fetchMock)).toBe(false); + expect(Object.keys(eventProps(fetchMock)).some((k) => k.startsWith("$feature/canary-"))).toBe( + false, + ); }); }); diff --git a/packages/cli/src/telemetry/client.ts b/packages/cli/src/telemetry/client.ts index 1df81c34df..a1af000eae 100644 --- a/packages/cli/src/telemetry/client.ts +++ b/packages/cli/src/telemetry/client.ts @@ -3,7 +3,7 @@ import { VERSION } from "../version.js"; import { c } from "../ui/colors.js"; import { diag } from "../ui/diagnostics.js"; import { getSystemMeta } from "./system.js"; -import { activeCanaryNames } from "./canary.js"; +import { canaryEventProperties } from "./canary.js"; import { enqueue, type EventProperties } from "./transport.js"; import { telemetryRuntimeOverride } from "./policy.js"; @@ -80,12 +80,13 @@ export function trackEvent( // we already knew. Absent (not false) when the config predates the // marker. Resolved after the shouldTrack guard. install_predecessor_found: readConfig().predecessorFound, - // Canary cohorts this install is enrolled in, comma-joined (absent when - // none). Attached to EVERY event, not just renders: a staged rollout is - // only as good as the ability to split any metric by cohort. Resolved - // after the shouldTrack guard above, so opted-out installs never pay for - // it. See telemetry/canary.ts. - canaries: activeCanaryNames(), + // Canary assignments as `$feature/canary-` — PostHog's native flag + // property shape, so breakdowns and experiment analysis work on a canary + // with nothing configured server-side. On EVERY event, not just renders: + // a staged rollout is only as good as the ability to split any metric by + // cohort. Resolved after the shouldTrack guard, so opted-out installs + // never pay for it. See telemetry/canary.ts. + ...canaryEventProperties(), agent_env_hints: sys.agent_env_hints ?? undefined, }, distinctId, diff --git a/packages/core/src/canary.test.ts b/packages/core/src/canary.test.ts index 66a3a6a509..78b038c325 100644 --- a/packages/core/src/canary.test.ts +++ b/packages/core/src/canary.test.ts @@ -2,6 +2,7 @@ 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"; const base = (over: Partial = {}): CanaryInput => ({ feature: "test-feature", @@ -249,3 +250,36 @@ describe("registry", () => { expect(overdueCanaries()).toEqual([]); }); }); + +describe("PostHog flag-shaped properties", () => { + it("namespaces keys so a canary can never alias a real PostHog flag", () => { + // A real flag namespace already exists in this project, owned by the web + // app (e.g. `enable-chat-tab`). Without the `canary-` infix a canary named + // after a real flag would fight it for the same property. + expect(canaryFeatureKey("de-parallel-router")).toBe("$feature/canary-de-parallel-router"); + expect(CANARY_FEATURE_PREFIX.startsWith("$feature/")).toBe(true); + }); + + it("emits every canary, not just enrolled ones", () => { + // Absent vs "false" are different facts: absent = this build predates the + // canary, "false" = this build has it and this install is control. + // Collapsing them makes a ramp unreadable. + const props = canaryFeatureProperties([ + { name: "a", enabled: true }, + { name: "b", enabled: false }, + ]); + expect(props).toEqual({ + "$feature/canary-a": "true", + "$feature/canary-b": "false", + }); + }); + + it("uses string values, matching how PostHog records boolean flags", () => { + const props = canaryFeatureProperties([{ name: "a", enabled: true }]); + expect(typeof props["$feature/canary-a"]).toBe("string"); + }); + + it("is empty when nothing is registered", () => { + expect(canaryFeatureProperties([])).toEqual({}); + }); +}); diff --git a/packages/core/src/canary.ts b/packages/core/src/canary.ts index 0b00e06417..e966423386 100644 --- a/packages/core/src/canary.ts +++ b/packages/core/src/canary.ts @@ -134,3 +134,47 @@ export function parseCanaryOverride(raw: string | undefined): boolean | undefine if (v === "0" || v === "false" || v === "off" || v === "no") return false; return undefined; } + +/** + * Property-name prefix for canary assignments on telemetry events. + * + * PostHog treats `$feature/` as a first-class flag property: breakdowns, + * funnels split by cohort and the experiment surfaces all key on it. Emitting + * assignments in that shape means the analysis tooling works on a canary with + * nothing configured server-side — the decision still happens locally and + * offline, which the render path requires (no render-time network calls, and + * behaviour must not depend on analytics being reachable). + * + * The `canary-` infix is deliberate. A real PostHog flag namespace already + * exists in this project, owned by the web app (e.g. `enable-chat-tab`, set by + * posthog-js). Namespacing guarantees a canary key can never alias a real flag + * key and have the two fight over the same property. + */ +export const CANARY_FEATURE_PREFIX = "$feature/canary-"; + +/** `de-parallel-router` → `$feature/canary-de-parallel-router`. */ +export function canaryFeatureKey(name: string): string { + return `${CANARY_FEATURE_PREFIX}${name}`; +} + +/** + * Build the telemetry properties for a set of resolved canaries. + * + * Emits EVERY registered canary, not just the enrolled ones, because absent + * and `"false"` mean different things: absent is "this build predates the + * canary", `"false"` is "this build has it and this install is not enrolled". + * Collapsing those makes a ramp unreadable — you cannot tell a control group + * from an old version. + * + * Values are the strings `"true"` / `"false"` to match how PostHog records + * boolean flag values, so the property is directly comparable to a real flag. + */ +export function canaryFeatureProperties( + entries: ReadonlyArray<{ name: string; enabled: boolean }>, +): Record { + const props: Record = {}; + for (const entry of entries) { + props[canaryFeatureKey(entry.name)] = entry.enabled ? "true" : "false"; + } + return props; +} diff --git a/packages/studio/src/telemetry/canary.test.ts b/packages/studio/src/telemetry/canary.test.ts index ee91abba01..5ae4077215 100644 --- a/packages/studio/src/telemetry/canary.test.ts +++ b/packages/studio/src/telemetry/canary.test.ts @@ -32,7 +32,7 @@ vi.mock("@hyperframes/core/canary-registry", async () => { const { isCanaryEnabled, resolveCanary, - activeCanaryNames, + canaryEventProperties, canaryParamName, __resetStudioCanaryCacheForTests, } = await import("./canary"); @@ -164,11 +164,14 @@ describe("cohort identity", () => { }); describe("telemetry", () => { - it("reports enrolled canaries, undefined when none", () => { - expect(activeCanaryNames()).toBe("on-everywhere"); + it("emits the same PostHog flag-shaped properties as the CLI", () => { + expect(canaryEventProperties()).toEqual({ + "$feature/canary-on-everywhere": "true", + "$feature/canary-off-everywhere": "false", + }); __resetStudioCanaryCacheForTests(); setSearch("?hf_canary_on_everywhere=off"); - expect(activeCanaryNames()).toBeUndefined(); + expect(canaryEventProperties()["$feature/canary-on-everywhere"]).toBe("false"); }); }); diff --git a/packages/studio/src/telemetry/canary.ts b/packages/studio/src/telemetry/canary.ts index f1494390e4..bf8b13905b 100644 --- a/packages/studio/src/telemetry/canary.ts +++ b/packages/studio/src/telemetry/canary.ts @@ -33,7 +33,12 @@ // browser bundle, and the barrel re-exports the whole core surface (parsers, // lint, studio-server); pulling that in here drags a Node-oriented dependency // graph into the bundle. These two modules are pure and leaf. -import { evaluateCanary, parseCanaryOverride, type CanaryDecision } from "@hyperframes/core/canary"; +import { + canaryFeatureProperties, + evaluateCanary, + parseCanaryOverride, + type CanaryDecision, +} from "@hyperframes/core/canary"; import { CANARIES, findCanary } from "@hyperframes/core/canary-registry"; import { resolveStudioDistinctId } from "./distinctId"; import { safeSessionStorage } from "../utils/safeStorage"; @@ -142,11 +147,12 @@ export function isCanaryEnabled(name: string): boolean { } /** - * Comma-joined names of the canaries this install is enrolled in, or undefined - * when none — attached to every Studio event so any metric can be split by - * cohort, exactly as the CLI does. + * Canary assignments as PostHog flag properties (`$feature/canary-`), + * attached to every Studio event so any metric can be split by cohort — + * identical shape to the CLI, so a rollout spanning both reads as one flag. */ -export function activeCanaryNames(): string | undefined { - const active = CANARIES.filter((c) => resolveCanary(c.name).enabled).map((c) => c.name); - return active.length > 0 ? active.join(",") : undefined; +export function canaryEventProperties(): Record { + return canaryFeatureProperties( + CANARIES.map((c) => ({ name: c.name, enabled: resolveCanary(c.name).enabled })), + ); } diff --git a/packages/studio/src/telemetry/client.ts b/packages/studio/src/telemetry/client.ts index b872ceb7a3..f43aa3486a 100644 --- a/packages/studio/src/telemetry/client.ts +++ b/packages/studio/src/telemetry/client.ts @@ -6,7 +6,7 @@ import { getAnonymousId, hasShownNotice, isOptedOut, markNoticeShown } from "./config"; import { getBrowserSystemMeta } from "./system"; -import { activeCanaryNames } from "./canary"; +import { canaryEventProperties } from "./canary"; // Write-only PostHog project key, safe to embed in client code. const POSTHOG_API_KEY = "phc_zjjbX0PnWxERXrMHhkEJWj9A9BhGVLRReICgsfTMmpx"; @@ -74,10 +74,10 @@ export function trackEvent(event: string, properties: EventProperties = {}): voi const sys = getBrowserSystemMeta(); eventQueue.push({ event, - // `canaries` mirrors the CLI: the cohorts this install is enrolled in, on - // EVERY event so any metric can be split by cohort. Resolved after the - // shouldTrack guard, so opted-out users never pay for it. - properties: { ...properties, ...sys, canaries: activeCanaryNames() }, + // Canary assignments as `$feature/canary-`, mirroring the CLI so a + // rollout spanning both surfaces reads as one flag in PostHog. Resolved + // after the shouldTrack guard, so opted-out users never pay for it. + properties: { ...properties, ...sys, ...canaryEventProperties() }, timestamp: new Date().toISOString(), }); From a7bb061afeaec9f39acc04314a673a2b2ded5fc2 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 11:17:51 -0700 Subject: [PATCH 05/20] feat(core): inert calibration canaries to validate the mechanism in the wild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Registers two canaries that gate nothing — `calibration-10` (10%) and `calibration-50` (50%) — so the rollout mechanism can be proven against real traffic before any real feature depends on it. Zero behavioural risk: they are read by nothing. They answer what the unit tests structurally cannot. The tests bucket generated UUIDs and weight every install equally; real render volume is heavily skewed toward a few heavy installs, and real install ids churn (~25x more distinct ids over 30 days than in any single day on the desktop render population). Four checks, pre-registered in the docs so the read is not post-hoc: 1. ACCURACY — does 10% land at 10%, install-weighted AND event-weighted? 2. DRIFT — how fast does CUMULATIVE exposure climb above target as ids churn? The instantaneous share is flat by construction; the set of installs enrolled at some point is not. 3. STABILITY — does any install ever change cohort? Must be zero. Percentages are held FIXED for the window precisely so a flip is unambiguously a bug; during a real ramp a false->true flip would be correct instead. 4. CROSS-SURFACE — do the CLI and Studio bindings agree for the same install? A CLI-launched Studio adopts the CLI id, and 16,961 installs currently share an id across both surfaces, so this is measurable. Plus an independence check: overlap between the two calibration canaries should be ~p1*p2 (~5%), not ~min(p1,p2) (~10%, which would mean every canary lands on the same unlucky cohort). The docs also record what calibration CANNOT fix: per-install cohorts never flip, but a person who wipes their config gets a new id and a fresh roll. Preventing that needs stable identity across resets, and both candidates were rejected — hardware fingerprinting correlates the cohort with hardware (fatal for a rendering experiment, and it survives uninstall) and account identity covers only ~3.6% of local rendering installs. The drift is therefore a measured, accepted limit, and the point of calibrating is to size it and pick canary window lengths accordingly. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 128 ++++++++++++++++++++++++++ packages/core/src/canaryRegistry.ts | 35 +++++++ 2 files changed, 163 insertions(+) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index 4e59fda7f0..720a86bdb5 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -83,6 +83,134 @@ Two details worth knowing: already exists in this project, owned by the web app. The infix guarantees a canary can never alias a real flag and fight it for the same property. +## Cumulative exposure — the number that actually bounds blast radius + +The instantaneous share holds at the target forever: enrolment is a pure +function of `(feature, installId, percentage)` and fresh ids are uniformly +random, so ~10% of active installs and ~10% of renders are enrolled on any +given day. That part does not drift. + +**The cumulative set does grow.** Install ids churn — measured on the desktop +render population, there are ~25× more distinct ids over 30 days than in any +single day. Ten percent of a pool that keeps turning over is a steadily larger +group of installs that have been enrolled *at some point*. If a single person +cycles through N ids during a rollout, their chance of having been exposed is +`1 − (1 − p)^N`, so at 10%: 19% after two ids, 41% after five. + +So watch the cumulative number, not just the rate: + +```sql +-- blast radius: distinct installs EVER enrolled during the window +SELECT + uniqExactIf(distinct_id, properties['$feature/canary-my-feature'] = 'true') AS ever_enrolled, + uniqExact(distinct_id) AS all_installs +FROM events +WHERE event = 'render_complete' AND timestamp >= now() - INTERVAL 14 DAY +``` + +Two practical consequences: + +- **Keep canaries short.** Drift compounds with time; a 5-day window at 10% is + far tighter than a 60-day one. +- **The percentage is not the safety mechanism.** It bounds *initial* exposure + and decays from there. Per-render verification and per-install circuit + breakers are what actually bound harm — and note that a breaker's state + lives in the same config file as the id, so a wipe loses both and the + install can re-enrol into a path that already failed it. + +For *measurement* — "is this feature better?" — churn is harmless: re-bucketing +is random, so it adds noise, not bias. It is specifically the blast-radius +guarantee that degrades. + +## Calibration: validating the mechanism in the wild + +Two inert canaries — `calibration-10` (10%) and `calibration-50` (50%) — gate +nothing and exist only to prove the mechanism behaves as designed on real +traffic. Their percentages stay FIXED for the whole window, which is what +makes check 3 below meaningful: while a percentage is constant, a cohort flip +is a bug, whereas during a real ramp a `false → true` flip is correct and +expected. + +Four checks, written down before the data arrives so the read is not post-hoc. + +**1. Accuracy — does 10% mean 10%?** Measure install-weighted AND +event-weighted separately: render volume is heavily skewed toward a few heavy +installs, so the two can differ even when bucketing is perfect. + +```sql +SELECT + properties['$feature/canary-calibration-10'] AS cohort, + uniqExact(distinct_id) AS installs, + count() AS events +FROM events +WHERE timestamp >= now() - INTERVAL 14 DAY + AND JSONHas(properties, '$feature/canary-calibration-10') +GROUP BY cohort +``` + +**2. Drift — does cumulative exposure climb?** Run over widening windows. The +instantaneous share should stay flat at 10%; the cumulative enrolled-install +count should grow with id churn. + +```sql +SELECT + uniqExactIf(distinct_id, properties['$feature/canary-calibration-10'] = 'true') AS ever_enrolled, + uniqExact(distinct_id) AS all_installs +FROM events WHERE timestamp >= now() - INTERVAL {1,7,14,30} DAY + AND JSONHas(properties, '$feature/canary-calibration-10') +``` + +**3. Stability — does any install ever change cohort?** MUST be zero. A single +id reporting both `true` and `false` at a fixed percentage means something is +broken: memoization, a registry edit mid-window, or two code paths +disagreeing. + +```sql +SELECT count() AS installs_that_flipped FROM ( + SELECT distinct_id + FROM events + WHERE timestamp >= now() - INTERVAL 14 DAY + AND JSONHas(properties, '$feature/canary-calibration-10') + GROUP BY distinct_id + HAVING uniqExact(properties['$feature/canary-calibration-10']) > 1 +) +``` + +**4. Cross-surface agreement — do CLI and Studio agree for the same install?** +A CLI-launched Studio adopts the CLI's id, so the same install must report the +same cohort on both. Disagreement means the two bindings have diverged. + +```sql +SELECT count() AS installs_disagreeing FROM ( + SELECT distinct_id + FROM events + WHERE timestamp >= now() - INTERVAL 14 DAY + AND JSONHas(properties, '$feature/canary-calibration-10') + GROUP BY distinct_id + HAVING uniqExact(startsWith(event, 'studio')) > 1 -- seen on both surfaces + AND uniqExact(properties['$feature/canary-calibration-10']) > 1 +) +``` + +**Independence bonus:** overlap between `calibration-10` and `calibration-50` +should be ~5% of installs (p1 x p2), not ~10% (which would mean the slices are +correlated and every canary hits the same unlucky cohort). + +### What passing looks like, and what cannot be fixed + +Checks 1, 3 and 4 should pass outright — they are properties of the design, +and failing any of them is a bug to fix before shipping a real rollout. + +Check 2 will NOT come back flat, and that is expected rather than a defect. +Per-install cohorts never flip, but a *person* who wipes their config gets a +new id and a fresh roll of the dice. Preventing that needs a stable identity +across resets, and both candidates were rejected: hardware fingerprinting +correlates the cohort with hardware (fatal for a rendering experiment, and it +survives uninstall), and account identity covers only ~3.6% of local +rendering installs. So the drift is a measured, accepted limit — the reason to +run this calibration is to learn its real magnitude and pick canary window +lengths accordingly. + ## Behaviour worth knowing **Ramping is inclusive.** Widening `10 → 25` keeps everyone who was already diff --git a/packages/core/src/canaryRegistry.ts b/packages/core/src/canaryRegistry.ts index b0b6dba87f..9373b78b71 100644 --- a/packages/core/src/canaryRegistry.ts +++ b/packages/core/src/canaryRegistry.ts @@ -45,6 +45,41 @@ export interface CanaryDefinition { } export const CANARIES: readonly CanaryDefinition[] = [ + // ── Calibration ────────────────────────────────────────────────────────── + // Two INERT canaries that gate nothing. They exist to validate the rollout + // mechanism against real traffic before anything real depends on it, and + // they answer questions the synthetic tests cannot: + // + // 1. Does a requested percentage land on target in the wild? The unit + // tests use generated UUIDs and weight every install equally; real + // render volume is heavily skewed toward a few heavy installs, so the + // render-weighted share could differ from the install-weighted one. + // 2. How fast does CUMULATIVE exposure drift above the target? Install + // ids churn (measured: 24.7x more distinct ids over 30 days than in + // any single day), so the set of installs enrolled AT SOME POINT grows + // even though the instantaneous share stays flat. That drift is the + // real limit on a canary's blast-radius guarantee. + // 3. Are two canaries actually independent on real ids, not just on + // generated ones? Overlap should be ~p1*p2, not ~min(p1,p2). + // + // Two different percentages so the answer is a line, not a point. + // Delete both once the calibration window is read. + { + name: "calibration-10", + percentage: 10, + description: "Inert. Validates rollout accuracy and cumulative-exposure drift at 10%.", + owner: "vance", + sunsetAfter: "2026-09-15", + }, + { + name: "calibration-50", + percentage: 50, + description: + "Inert. Second calibration point, and an independence check against calibration-10.", + owner: "vance", + sunsetAfter: "2026-09-15", + }, + // ── Real rollouts ──────────────────────────────────────────────────────── { name: "de-parallel-router", percentage: 0, From 00b57629746d3006d8cf3d8fc760b7e0e84c4f77 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 11:28:22 -0700 Subject: [PATCH 06/20] docs(canary): link the calibration dashboard from the checks section The four pre-registered checks now exist as PostHog tiles. Without the link the doc describes queries someone has to re-type; with it the doc and the dashboard are one artifact. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index 720a86bdb5..cc9574aaf5 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -132,6 +132,9 @@ is a bug, whereas during a real ramp a `false → true` flip is correct and expected. Four checks, written down before the data arrives so the read is not post-hoc. +All four are live as tiles on the [Canary rollout calibration +dashboard](https://us.posthog.com/project/356858/dashboard/1918875) — the SQL +below is what each tile runs. **1. Accuracy — does 10% mean 10%?** Measure install-weighted AND event-weighted separately: render volume is heavily skewed toward a few heavy From 6f6f01c8ea3efa011ac2e6f43035a8bbd037eaf0 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 11:39:10 -0700 Subject: [PATCH 07/20] docs(canary): correct the loose-toggle count in the registry header Verified against packages/*/src env reads: 57 distinct HF_*/PRODUCER_* toggles pre-existing this branch (the raw grep said 59, but two of those are HF_CANARY_TEST_* fixtures introduced by this branch's own tests). The number is cited externally now, so it should match what the repo actually has. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/canaryRegistry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/canaryRegistry.ts b/packages/core/src/canaryRegistry.ts index 9373b78b71..d74a2b1394 100644 --- a/packages/core/src/canaryRegistry.ts +++ b/packages/core/src/canaryRegistry.ts @@ -2,7 +2,7 @@ * The canary registry — every staged rollout in the product, in one file. * * Why a registry rather than a percentage inlined at each call site: the repo - * already carries ~49 loose `HF_*` / `PRODUCER_*` toggles with no index, so + * already carries 57 loose `HF_*` / `PRODUCER_*` toggles with no index, so * nobody can answer "what is currently rolling out, to how many people, and * who owns it" without grepping. One table fixes that, and gives the * telemetry and `doctor` surfaces something to enumerate. From c43b4e2d951c229935f65a4e224ef8a66c43dd12 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 12:04:28 -0700 Subject: [PATCH 08/20] docs(canary): make calibration check 4 compare per-surface values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As written, check 4's condition (dual-surface AND value flipped anywhere) was a strict subset of check 3's (value flipped) — if check 3 read zero, check 4 was vacuously zero and added no independent signal. Compare the value each surface actually reported instead, and state plainly that any disagreement is also a check-3 flip: this check's job is attributing such a flip to binding divergence. Matching fix applied to the live PostHog tile (insight jQi7QdW1, dashboard 1918875). Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index cc9574aaf5..f3cf7f5763 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -181,17 +181,23 @@ SELECT count() AS installs_that_flipped FROM ( **4. Cross-surface agreement — do CLI and Studio agree for the same install?** A CLI-launched Studio adopts the CLI's id, so the same install must report the -same cohort on both. Disagreement means the two bindings have diverged. +same cohort on both. Disagreement means the two bindings have diverged. Note +this compares the *value reported per surface* — any disagreement here is also +a check-3 flip, so this check's job is attribution: it isolates the flips that +are binding divergence rather than within-surface instability. ```sql -SELECT count() AS installs_disagreeing FROM ( - SELECT distinct_id +SELECT countIf(cli_val != studio_val) AS installs_disagreeing FROM ( + SELECT + distinct_id, + anyIf(properties['$feature/canary-calibration-10'], NOT startsWith(event, 'studio')) AS cli_val, + anyIf(properties['$feature/canary-calibration-10'], startsWith(event, 'studio')) AS studio_val FROM events WHERE timestamp >= now() - INTERVAL 14 DAY AND JSONHas(properties, '$feature/canary-calibration-10') GROUP BY distinct_id - HAVING uniqExact(startsWith(event, 'studio')) > 1 -- seen on both surfaces - AND uniqExact(properties['$feature/canary-calibration-10']) > 1 + HAVING countIf(startsWith(event, 'studio')) > 0 -- seen on both surfaces + AND countIf(NOT startsWith(event, 'studio')) > 0 ) ``` From 10536e8473b31eef5b35a2977673bcae35be83c5 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 13:14:58 -0700 Subject: [PATCH 09/20] docs(canary): note override contamination as check 3's innocent explanation Telemetry carries the assignment but not the decision reason, so a manual HF_CANARY_* toggle mid-window reads as a cohort flip. Rule it out before treating a small-nonzero stability read as a mechanism bug; a reason property is deliberately deferred until the check actually comes back dirty. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index f3cf7f5763..34f566a681 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -166,7 +166,11 @@ FROM events WHERE timestamp >= now() - INTERVAL {1,7,14,30} DAY **3. Stability — does any install ever change cohort?** MUST be zero. A single id reporting both `true` and `false` at a fixed percentage means something is broken: memoization, a registry edit mid-window, or two code paths -disagreeing. +disagreeing. One innocent explanation to rule out first on a small-nonzero +read: events carry the assignment but not the decision *reason*, so a dev +toggling `HF_CANARY_CALIBRATION_10=on/off` mid-window is indistinguishable +from a real flip. Emitting a reason property is deliberately skipped — add it +only if this check comes back dirty. ```sql SELECT count() AS installs_that_flipped FROM ( From 18d6dd2d7ff44994d24d360edd0d7981967fd29d Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 13:36:02 -0700 Subject: [PATCH 10/20] docs(canary): keep contributor docs vendor- and access-neutral Contributors in the wild have no access to the project's analytics backend, so the contributing doc must not point them at it: drop the internal dashboard link, the backend-specific SQL blocks, and the vendor naming. The calibration check definitions stay (public, fixed terms of the experiment); the queries live with the dashboard tiles that run them. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 105 ++++++-------------------- 1 file changed, 24 insertions(+), 81 deletions(-) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index 34f566a681..cd501431c4 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -57,21 +57,18 @@ panic-off. ## Measuring -Every telemetry event carries the assignment as a PostHog flag property: +Every telemetry event carries the assignment as a flag-shaped property: ``` $feature/canary-my-feature: "true" | "false" ``` -PostHog treats `$feature/` as a first-class flag, so breakdowns, funnels -split by cohort and the experiment surfaces work on a canary with **nothing -configured server-side** — the decision still happens locally and offline, -which the render path requires. - -```sql -SELECT properties['$feature/canary-my-feature'] AS cohort, count() -FROM events WHERE event = 'render_complete' GROUP BY cohort -``` +`$feature/` is the de-facto flag-property convention in analytics +tooling, so whoever operates the telemetry backend can split any metric by +cohort with **nothing configured server-side** — while the decision itself +still happens locally and offline, which the render path requires. +(Assignments ride the same anonymous, opt-out telemetry pipeline as every +other event; disabling telemetry disables the reporting, not the enrolment.) Two details worth knowing: @@ -79,9 +76,10 @@ Two details worth knowing: missing property. Absent means *this build predates the canary*, which is a different fact from *this install is control* — collapsing them makes a ramp unreadable. -- **Keys are namespaced with `canary-`.** A real PostHog flag namespace - already exists in this project, owned by the web app. The infix guarantees a - canary can never alias a real flag and fight it for the same property. +- **Keys are namespaced with `canary-`.** The `$feature/` namespace is shared + with real product feature flags elsewhere in the analytics pipeline. The + infix guarantees a canary can never alias one and fight it for the same + property. ## Cumulative exposure — the number that actually bounds blast radius @@ -97,18 +95,8 @@ group of installs that have been enrolled *at some point*. If a single person cycles through N ids during a rollout, their chance of having been exposed is `1 − (1 − p)^N`, so at 10%: 19% after two ids, 41% after five. -So watch the cumulative number, not just the rate: - -```sql --- blast radius: distinct installs EVER enrolled during the window -SELECT - uniqExactIf(distinct_id, properties['$feature/canary-my-feature'] = 'true') AS ever_enrolled, - uniqExact(distinct_id) AS all_installs -FROM events -WHERE event = 'render_complete' AND timestamp >= now() - INTERVAL 14 DAY -``` - -Two practical consequences: +So the number that bounds blast radius is *distinct installs ever enrolled +during the window*, not the instantaneous rate. Two practical consequences: - **Keep canaries short.** Drift compounds with time; a 5-day window at 10% is far tighter than a 60-day one. @@ -132,36 +120,17 @@ is a bug, whereas during a real ramp a `false → true` flip is correct and expected. Four checks, written down before the data arrives so the read is not post-hoc. -All four are live as tiles on the [Canary rollout calibration -dashboard](https://us.posthog.com/project/356858/dashboard/1918875) — the SQL -below is what each tile runs. +(The maintainers monitor these on an internal dashboard; the definitions live +here so the experiment's terms are public and fixed.) -**1. Accuracy — does 10% mean 10%?** Measure install-weighted AND +**1. Accuracy — does 10% mean 10%?** Measured install-weighted AND event-weighted separately: render volume is heavily skewed toward a few heavy -installs, so the two can differ even when bucketing is perfect. - -```sql -SELECT - properties['$feature/canary-calibration-10'] AS cohort, - uniqExact(distinct_id) AS installs, - count() AS events -FROM events -WHERE timestamp >= now() - INTERVAL 14 DAY - AND JSONHas(properties, '$feature/canary-calibration-10') -GROUP BY cohort -``` +installs, so the two can differ even when bucketing is perfect. Install share +is the one that must land on target. -**2. Drift — does cumulative exposure climb?** Run over widening windows. The -instantaneous share should stay flat at 10%; the cumulative enrolled-install -count should grow with id churn. - -```sql -SELECT - uniqExactIf(distinct_id, properties['$feature/canary-calibration-10'] = 'true') AS ever_enrolled, - uniqExact(distinct_id) AS all_installs -FROM events WHERE timestamp >= now() - INTERVAL {1,7,14,30} DAY - AND JSONHas(properties, '$feature/canary-calibration-10') -``` +**2. Drift — does cumulative exposure climb?** Measured over widening windows +(1/7/14/30 days). The instantaneous share should stay flat at 10%; the +cumulative enrolled-install count should grow with id churn. **3. Stability — does any install ever change cohort?** MUST be zero. A single id reporting both `true` and `false` at a fixed percentage means something is @@ -172,39 +141,13 @@ toggling `HF_CANARY_CALIBRATION_10=on/off` mid-window is indistinguishable from a real flip. Emitting a reason property is deliberately skipped — add it only if this check comes back dirty. -```sql -SELECT count() AS installs_that_flipped FROM ( - SELECT distinct_id - FROM events - WHERE timestamp >= now() - INTERVAL 14 DAY - AND JSONHas(properties, '$feature/canary-calibration-10') - GROUP BY distinct_id - HAVING uniqExact(properties['$feature/canary-calibration-10']) > 1 -) -``` - **4. Cross-surface agreement — do CLI and Studio agree for the same install?** A CLI-launched Studio adopts the CLI's id, so the same install must report the -same cohort on both. Disagreement means the two bindings have diverged. Note -this compares the *value reported per surface* — any disagreement here is also -a check-3 flip, so this check's job is attribution: it isolates the flips that +same cohort on both. Disagreement means the two bindings have diverged. This +compares the *value reported per surface* — any disagreement here is also a +check-3 flip, so this check's job is attribution: it isolates the flips that are binding divergence rather than within-surface instability. -```sql -SELECT countIf(cli_val != studio_val) AS installs_disagreeing FROM ( - SELECT - distinct_id, - anyIf(properties['$feature/canary-calibration-10'], NOT startsWith(event, 'studio')) AS cli_val, - anyIf(properties['$feature/canary-calibration-10'], startsWith(event, 'studio')) AS studio_val - FROM events - WHERE timestamp >= now() - INTERVAL 14 DAY - AND JSONHas(properties, '$feature/canary-calibration-10') - GROUP BY distinct_id - HAVING countIf(startsWith(event, 'studio')) > 0 -- seen on both surfaces - AND countIf(NOT startsWith(event, 'studio')) > 0 -) -``` - **Independence bonus:** overlap between `calibration-10` and `calibration-50` should be ~5% of installs (p1 x p2), not ~10% (which would mean the slices are correlated and every canary hits the same unlucky cohort). From 0d98de023f5d38b0f7767c7f4defc6a85bff217a Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 14:36:42 -0700 Subject: [PATCH 11/20] docs(canary): calibration check 2 reflects the breaker rollover The "what cannot be fixed" framing predated the state-file rollover: the drift still exists, but a re-rolled install no longer re-enters a failed path, and install_predecessor_found splits the drift into recoverable vs unrecoverable. Window-length policy comes from the residual, not raw turnover. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index cd501431c4..0db264ee65 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -159,13 +159,26 @@ and failing any of them is a bug to fix before shipping a real rollout. Check 2 will NOT come back flat, and that is expected rather than a defect. Per-install cohorts never flip, but a *person* who wipes their config gets a -new id and a fresh roll of the dice. Preventing that needs a stable identity -across resets, and both candidates were rejected: hardware fingerprinting -correlates the cohort with hardware (fatal for a rendering experiment, and it -survives uninstall), and account identity covers only ~3.6% of local -rendering installs. So the drift is a measured, accepted limit — the reason to -run this calibration is to learn its real magnitude and pick canary window -lengths accordingly. +new id and a fresh roll of the dice. Preventing that entirely needs a stable +identity across resets, and both candidates were rejected: hardware +fingerprinting correlates the cohort with hardware (fatal for a rendering +experiment, and it survives uninstall), and account identity covers only +~3.6% of local rendering installs. So the drift itself stands as a measured, +accepted limit — but its worst consequence is mitigated, and its size is now +directly measurable rather than inferred: + +- **A re-rolled install cannot re-enter a path that already failed on that + machine.** The circuit breaker's tripped state is mirrored to the + machine-local state file, so it survives the same config wipe that re-rolls + the cohort. +- **`install_predecessor_found`** on every event says whether this install's + mint found a previous install's state marker. Its true-share splits check + 2's drift into the recoverable part (config wiped, machine persisted) and + the part no local mechanism can link (fresh machine, container, new user). + +The calibration read is therefore two numbers — total drift, and the fraction +the rollover already covers — and canary window lengths get picked from the +residual, not the raw turnover. ## Behaviour worth knowing From 8dd20ac2e8f8c082c69ac9a4be5f3df8b7690b2c Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 16:57:23 -0700 Subject: [PATCH 12/20] feat(cli): bucket canaries on a machine-lineage seed, not the telemetry id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cohort membership now survives a config wipe. Canaries bucket on a dedicated bucketSeed (fresh random UUID, distinct from anonymousId by design) that is mirrored write-once into the install-state file and inherited at mint: a wipe re-rolls the telemetry id but never the canary assignment. This removes cumulative-exposure drift for the recoverable churn bucket entirely — the residual drift comes only from fresh machines, containers, and genuinely new users — and keeps before/after comparisons valid across a reinstall. The seed is never emitted in telemetry (only the resulting true/false assignments are), so it does not link the old id to the new one server-side. The residual linker is the flag vector itself (k bits for k live canaries), documented as such. An explicit reset still works by deleting the state file, and the no-identity test now also asserts the seed differs from the anonymousId. Cross-surface coherence: the CLI's studio server injects the seed as window.__HF_CLI_BUCKET_SEED (same telemetry gate and script-escaping as the distinct id, and on the /api/telemetry-identity fallback), and the Studio binding buckets on it when present — without this the CLI would bucket on the seed while Studio bucketed on the distinct id, splitting one machine across cohorts (calibration check 4 would catch exactly this). Standalone Studio still buckets on its localStorage id: the browser has no second storage location, so that id doubles as the seed. Legacy configs are backfilled once (lineage seed if the state file has one, else minted) and persisted immediately — an unpersisted seed would re-roll cohorts every process. Safe to ship in the same release as the first canaries: no prior release emitted canary properties, so the bucketing-unit change is unobservable. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 27 +++-- packages/cli/src/server/studioServer.ts | 11 +- .../cli/src/server/telemetryIdentity.test.ts | 8 ++ packages/cli/src/server/telemetryIdentity.ts | 36 +++++- packages/cli/src/telemetry/canary.test.ts | 40 ++++++- packages/cli/src/telemetry/canary.ts | 7 +- packages/cli/src/telemetry/config.test.ts | 78 ++++++++++++- packages/cli/src/telemetry/config.ts | 110 ++++++++++++++---- packages/studio/src/telemetry/canary.test.ts | 31 +++-- packages/studio/src/telemetry/canary.ts | 36 +++++- 10 files changed, 327 insertions(+), 57 deletions(-) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index 0db264ee65..4b7344dac0 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -167,18 +167,24 @@ experiment, and it survives uninstall), and account identity covers only accepted limit — but its worst consequence is mitigated, and its size is now directly measurable rather than inferred: +- **Cohorts survive the wipe.** Canaries bucket on a dedicated `bucketSeed` — + not the telemetry id — and the seed is inherited across config wipes via a + machine-local state file (write-once: the first install's seed is the + lineage's seed forever). A wipe re-rolls the telemetry id, never the canary + assignment. The seed is never emitted, so it does not link the old id to + the new one. - **A re-rolled install cannot re-enter a path that already failed on that - machine.** The circuit breaker's tripped state is mirrored to the - machine-local state file, so it survives the same config wipe that re-rolls - the cohort. + machine.** The circuit breaker's tripped state is mirrored to the same + state file. - **`install_predecessor_found`** on every event says whether this install's mint found a previous install's state marker. Its true-share splits check 2's drift into the recoverable part (config wiped, machine persisted) and the part no local mechanism can link (fresh machine, container, new user). -The calibration read is therefore two numbers — total drift, and the fraction -the rollover already covers — and canary window lengths get picked from the -residual, not the raw turnover. +With the seed carryover, check 2's residual drift comes from the +unrecoverable buckets only — fresh machines, containers, and genuinely new +users — and canary window lengths get picked from that residual, not the raw +turnover. ## Behaviour worth knowing @@ -186,11 +192,16 @@ residual, not the raw turnover. in the 10. Cohorts never reshuffle, so a before/after comparison stays valid across a ramp. -**Slices are independent per feature.** The bucket hashes `feature:installId`, -not the install id alone — two canaries at 10% select two different 10%s. If +**Slices are independent per feature.** The bucket hashes `feature:seed`, +not the seed alone — two canaries at 10% select two different 10%s. If they shared a slice, one unlucky cohort would receive every experiment at once and no two rollouts could be read apart. +**Cohorts are keyed to the machine, not the config.** The bucketing unit is a +dedicated seed inherited across config wipes (see the calibration section) — +distinct from the telemetry id, never emitted, and shared with a CLI-launched +Studio so both surfaces agree. + **It fails closed.** No install id, an unregistered name, or a CI machine all resolve to *not enrolled*. A canary exists to bound blast radius, so "we don't know who this is" must never mean "enrol everyone". CI is excluded diff --git a/packages/cli/src/server/studioServer.ts b/packages/cli/src/server/studioServer.ts index 358189ad63..9bf7e97429 100644 --- a/packages/cli/src/server/studioServer.ts +++ b/packages/cli/src/server/studioServer.ts @@ -16,7 +16,11 @@ import { loadRuntimeSourceSignature, } from "./runtimeSource.js"; import { VERSION as version } from "../version.js"; -import { buildStudioHeadScripts, resolveCliTelemetryDistinctId } from "./telemetryIdentity.js"; +import { + buildStudioHeadScripts, + resolveCliBucketSeed, + resolveCliTelemetryDistinctId, +} from "./telemetryIdentity.js"; import { emitStudioRenderComplete, emitStudioRenderError } from "./studioRenderTelemetry.js"; import { isDevMode } from "../utils/env.js"; import { @@ -652,7 +656,10 @@ export function createStudioServer(options: StudioServerOptions): StudioServer { // distinct id (no PII) so the browser session can join the CLI's PostHog // person, or `{ distinctId: null }` when CLI telemetry is disabled. app.get("/api/telemetry-identity", (c) => { - return c.json({ distinctId: resolveCliTelemetryDistinctId() }); + return c.json({ + distinctId: resolveCliTelemetryDistinctId(), + bucketSeed: resolveCliBucketSeed(), + }); }); app.get("/api/events", (c) => { diff --git a/packages/cli/src/server/telemetryIdentity.test.ts b/packages/cli/src/server/telemetryIdentity.test.ts index 9f779841fb..cd5535bd27 100644 --- a/packages/cli/src/server/telemetryIdentity.test.ts +++ b/packages/cli/src/server/telemetryIdentity.test.ts @@ -66,6 +66,14 @@ describe("buildCliIdentityScript", () => { ); }); + it("also seeds window.__HF_CLI_BUCKET_SEED when the config carries a bucket seed", () => { + shouldTrack.mockReturnValue(true); + readConfig.mockReturnValue({ anonymousId: "machine-uuid", bucketSeed: "seed-uuid" }); + expect(buildCliIdentityScript()).toBe( + '', + ); + }); + it("emits an empty string when telemetry is disabled (nothing to seed)", () => { shouldTrack.mockReturnValue(false); expect(buildCliIdentityScript()).toBe(""); diff --git a/packages/cli/src/server/telemetryIdentity.ts b/packages/cli/src/server/telemetryIdentity.ts index f5d4d36df0..9a24765e4b 100644 --- a/packages/cli/src/server/telemetryIdentity.ts +++ b/packages/cli/src/server/telemetryIdentity.ts @@ -41,15 +41,39 @@ export function resolveCliTelemetryDistinctId(): string | null { * `url_hash` telemetry or browser history. Empty string when there's nothing to * seed (telemetry off / no id). */ +/** + * The CLI's canary bucket seed to hand to Studio, or null. Injected alongside + * the distinct id so a CLI-launched Studio buckets canaries on the SAME unit + * as the CLI — without it the two surfaces would agree only while the seed + * still equals whatever Studio falls back to, and a rollout spanning render + * and editor would split one user across cohorts. Same telemetry gate as the + * distinct id: seeding is part of the identity stitch, not a separate channel. + */ +export function resolveCliBucketSeed(): string | null { + try { + if (!telemetryShouldTrack()) return null; + const seed = readConfig().bucketSeed; + return typeof seed === "string" && seed.length > 0 ? seed : null; + } catch { + return null; + } +} + +// JSON.stringify does not escape "<" or "/". Escaping both means no +// "" (or " or open a new tag. (The values are +// randomUUID()s, so this is belt-and-suspenders.) +function encodeInlineScriptValue(value: string): string { + return JSON.stringify(value).replace(/" (or " or open a new tag. - const encoded = JSON.stringify(cliId).replace(/window.__HF_CLI_DISTINCT_ID=${encoded};`; + const parts = [`window.__HF_CLI_DISTINCT_ID=${encodeInlineScriptValue(cliId)};`]; + const seed = resolveCliBucketSeed(); + if (seed) parts.push(`window.__HF_CLI_BUCKET_SEED=${encodeInlineScriptValue(seed)};`); + return ``; } /** diff --git a/packages/cli/src/telemetry/canary.test.ts b/packages/cli/src/telemetry/canary.test.ts index 0d5043170c..f1d8b8b10a 100644 --- a/packages/cli/src/telemetry/canary.test.ts +++ b/packages/cli/src/telemetry/canary.test.ts @@ -1,10 +1,13 @@ import { describe, expect, it, vi, beforeEach } from "vitest"; -const configState = { anonymousId: "db0c1f4a-b95e-4c35-90c6-1a15bd76f717" }; +const configState: { anonymousId: string; bucketSeed: string | undefined } = { + anonymousId: "db0c1f4a-b95e-4c35-90c6-1a15bd76f717", + bucketSeed: undefined, +}; const systemState = { is_ci: false }; vi.mock("./config.js", () => ({ - readConfig: () => ({ anonymousId: configState.anonymousId }), + readConfig: () => ({ anonymousId: configState.anonymousId, bucketSeed: configState.bucketSeed }), })); vi.mock("./system.js", () => ({ getSystemMeta: () => ({ is_ci: systemState.is_ci }), @@ -60,11 +63,44 @@ const { isCanaryEnabled, resolveCanary, canaryEventProperties, __resetCanaryCach beforeEach(() => { __resetCanaryCacheForTests(); configState.anonymousId = "db0c1f4a-b95e-4c35-90c6-1a15bd76f717"; + configState.bucketSeed = undefined; systemState.is_ci = false; delete process.env.HF_CANARY_TEST_ALPHA; delete process.env.HF_CANARY_TEST_BETA; }); +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 bySeed = evaluateCanary({ + feature: "test-alpha", + unitId: configState.bucketSeed, + percentage: 100, + }).bucket; + const byId = evaluateCanary({ + feature: "test-alpha", + unitId: configState.anonymousId, + percentage: 100, + }).bucket; + expect(viaBinding).toBe(bySeed); + // Only meaningful if the two units actually bucket differently. + expect(bySeed).not.toBe(byId); + }); + + 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 byId = evaluateCanary({ + feature: "test-alpha", + unitId: configState.anonymousId, + percentage: 100, + }).bucket; + expect(viaBinding).toBe(byId); + }); +}); + describe("CLI canary binding", () => { it("reads the percentage from the registry", () => { expect(isCanaryEnabled("test-alpha")).toBe(true); diff --git a/packages/cli/src/telemetry/canary.ts b/packages/cli/src/telemetry/canary.ts index 56d41ae7ee..49d86918ef 100644 --- a/packages/cli/src/telemetry/canary.ts +++ b/packages/cli/src/telemetry/canary.ts @@ -55,10 +55,15 @@ export function resolveCanary(name: string): CanaryDecision { if (cached) return cached; const definition = findCanary(name); + const config = readConfig(); const decision: CanaryDecision = definition ? evaluateCanary({ feature: definition.name, - unitId: readConfig().anonymousId, + // The bucket seed, NOT the anonymousId: the seed is inherited across + // config wipes via the install-state file, so the machine keeps its + // cohorts when the telemetry id re-rolls. Fallback covers only a + // failed backfill write on a legacy config. + unitId: config.bucketSeed ?? config.anonymousId, percentage: definition.percentage, override: parseCanaryOverride(process.env[canaryEnvVar(definition.name)]), // CI installs regenerate their config per run, so their ids are diff --git a/packages/cli/src/telemetry/config.test.ts b/packages/cli/src/telemetry/config.test.ts index 571ef736a5..646284eda6 100644 --- a/packages/cli/src/telemetry/config.test.ts +++ b/packages/cli/src/telemetry/config.test.ts @@ -204,11 +204,18 @@ describe("install-state rollover (breaker survives a config re-mint)", () => { expect(readConfigFresh().deParallelRouterTrialFired).toBe(true); }); - it("the state file holds no identity — only the marker timestamp and breaker fact", () => { + it("the state file holds no telemetry identity — marker, breaker fact, bucket seed only", () => { const config = readConfig(); config.deParallelRouterTrialFired = true; writeConfig(config); - expect(Object.keys(stateFile()).sort()).toEqual(["deParallelRouterTrialFired", "markerAt"]); + expect(Object.keys(stateFile()).sort()).toEqual([ + "bucketSeed", + "deParallelRouterTrialFired", + "markerAt", + ]); + // The seed is a separate random UUID, never the anonymousId — the old + // telemetry id must not survive a wipe in any form. + expect(config.bucketSeed).not.toBe(config.anonymousId); expect(JSON.stringify(stateFile())).not.toContain(config.anonymousId); }); @@ -292,3 +299,70 @@ describe("install-state rollover (breaker survives a config re-mint)", () => { expect(fsState.files.has(LEGACY_STATE_PATH)).toBe(false); }); }); + +describe("bucket-seed carryover (cohorts survive a config wipe)", () => { + let readConfig: typeof import("./config.js").readConfig; + let readConfigFresh: typeof import("./config.js").readConfigFresh; + let writeConfig: typeof import("./config.js").writeConfig; + 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, writeConfig, CONFIG_PATH, STATE_PATH } = + await import("./config.js")); + }); + + it("a fresh install mints a seed distinct from its anonymousId and mirrors it to the state file", () => { + const config = readConfig(); + expect(config.bucketSeed).toBeTruthy(); + expect(config.bucketSeed).not.toBe(config.anonymousId); + const state = JSON.parse(fsState.files.get(STATE_PATH) as string) as { bucketSeed?: string }; + expect(state.bucketSeed).toBe(config.bucketSeed); + }); + + it("the seed survives a config wipe — the machine keeps its cohorts, only the id re-rolls", () => { + const first = readConfig(); + fsState.files.delete(CONFIG_PATH); + const second = readConfigFresh(); + expect(second.bucketSeed).toBe(first.bucketSeed); + expect(second.anonymousId).not.toBe(first.anonymousId); + }); + + it("the seed survives config corruption via the same mint path", () => { + const first = readConfig(); + fsState.files.set(CONFIG_PATH, "{not valid json"); + expect(readConfigFresh().bucketSeed).toBe(first.bucketSeed); + }); + + it("the state-file seed is write-once — a later install never overwrites the lineage seed", () => { + const first = readConfig(); + fsState.files.delete(CONFIG_PATH); + const second = readConfigFresh(); + // Force more config writes from the second install; the state seed must not move. + second.commandCount = 7; + writeConfig(second); + const state = JSON.parse(fsState.files.get(STATE_PATH) as string) as { bucketSeed?: string }; + expect(state.bucketSeed).toBe(first.bucketSeed); + }); + + it("a legacy config without a seed is backfilled ONCE and stays stable across fresh reads", () => { + const base = readConfig(); + const { bucketSeed: _dropped, ...legacy } = base; + fsState.files.set(CONFIG_PATH, JSON.stringify(legacy)); + fsState.files.delete(STATE_PATH); // no lineage either — pure legacy machine + const first = readConfigFresh(); + expect(first.bucketSeed).toBeTruthy(); + const second = readConfigFresh(); + expect(second.bucketSeed).toBe(first.bucketSeed); // persisted, not re-rolled per read + }); + + it("a legacy config adopts the lineage seed from the state file when one exists", () => { + const base = readConfig(); // wrote the state file with a seed + const lineageSeed = base.bucketSeed; + const { bucketSeed: _dropped, ...legacy } = base; + fsState.files.set(CONFIG_PATH, JSON.stringify(legacy)); + expect(readConfigFresh().bucketSeed).toBe(lineageSeed); + }); +}); diff --git a/packages/cli/src/telemetry/config.ts b/packages/cli/src/telemetry/config.ts index 87831bec1d..23c254268e 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -56,6 +56,12 @@ interface InstallState { markerAt: string; /** Rolled-over circuit-breaker state — see HyperframesConfig's field. */ deParallelRouterTrialFired?: boolean; + /** + * The machine's canary bucketing seed — see HyperframesConfig's field. + * Write-once: the first install's seed is the lineage's seed forever, so a + * config wipe re-mints the telemetry id but NOT the canary cohorts. + */ + bucketSeed?: string; } /** Parse one state file; any parse/shape failure reads as absent. */ @@ -67,6 +73,10 @@ function parseInstallState(file: string): InstallState | null { return { markerAt: parsed.markerAt, deParallelRouterTrialFired: parsed.deParallelRouterTrialFired === true ? true : undefined, + bucketSeed: + typeof parsed.bucketSeed === "string" && parsed.bucketSeed.length > 0 + ? parsed.bucketSeed + : undefined, }; } catch { return null; @@ -136,18 +146,35 @@ function writeInstallState(next: InstallState): void { * no breaker write site can forget it. Never throws — same contract as the * rest of this file, telemetry must not break the CLI. */ +function sameInstallState(a: InstallState, b: InstallState): boolean { + return ( + a.deParallelRouterTrialFired === b.deParallelRouterTrialFired && a.bucketSeed === b.bucketSeed + ); +} + +/** Latching: once tripped (by any install in this machine's lineage), stays tripped. */ +function latchedFired(state: InstallState | null, config: HyperframesConfig): true | undefined { + return ( + state?.deParallelRouterTrialFired === true || + config.deParallelRouterTrialFired === true || + undefined + ); +} + /** What the state file should say after this config write; null = already correct. */ -function nextInstallState(state: InstallState | null, wantFired: boolean): InstallState | null { - const hadFired = state?.deParallelRouterTrialFired === true; - if (state !== null && (hadFired || !wantFired)) return null; - // Every path reaching here has hadFired === false (state is either null, or - // the guard above already returned when hadFired was true) — the field is - // simply wantFired, not a merge of the two (review nit, two independent - // reviewers). - return { +function nextInstallState( + state: InstallState | null, + config: HyperframesConfig, +): InstallState | null { + const next: InstallState = { markerAt: state?.markerAt ?? new Date().toISOString(), - deParallelRouterTrialFired: wantFired || undefined, + deParallelRouterTrialFired: latchedFired(state, config), + // Write-once: an existing lineage seed always wins, so cohorts stay + // anchored to the FIRST install in this config dir, not the latest one. + bucketSeed: state?.bucketSeed ?? config.bucketSeed, }; + if (state !== null && sameInstallState(state, next)) return null; + return next; } function syncInstallState(config: HyperframesConfig): void { @@ -155,7 +182,7 @@ function syncInstallState(config: HyperframesConfig): void { if (stateMarkerSynced && (stateFiredSynced || !wantFired)) return; try { const state = readInstallState(); - const next = nextInstallState(state, wantFired); + const next = nextInstallState(state, config); if (next !== null) writeInstallState(next); stateMarkerSynced = true; stateFiredSynced = wantFired || state?.deParallelRouterTrialFired === true; @@ -176,8 +203,11 @@ function mintConfig(): HyperframesConfig { anonymousId: randomUUID(), predecessorFound: state !== null, // The rollover itself: a breaker tripped by a previous install on this - // machine stays tripped for the new one. + // machine stays tripped for the new one, and the canary bucketing seed is + // inherited so the machine keeps its cohorts — a wipe re-rolls the + // telemetry id, never the canary assignment. deParallelRouterTrialFired: state?.deParallelRouterTrialFired === true ? true : undefined, + bucketSeed: state?.bucketSeed ?? randomUUID(), }; } @@ -264,6 +294,17 @@ export interface HyperframesConfig { * existed — a different fact from `false` (minted fresh, no predecessor). */ predecessorFound?: boolean; + /** + * The unit canary percentages bucket on — deliberately NOT the anonymousId. + * A fresh random UUID, mirrored write-once into the install-state file and + * inherited at mint, so a config wipe re-rolls the telemetry id but keeps + * the machine's canary cohorts: no cumulative-exposure drift from wipes, + * and before/after comparisons survive a reinstall. It is never emitted in + * telemetry (only the resulting true/false assignments are), so it does not + * link the old id to the new one server-side. Backfilled once for configs + * predating the field. + */ + bucketSeed?: string; /** * Ring of the last few local renders (newest last). `hyperframes feedback` * attaches these ids — which are the `render_job_id` / @@ -310,6 +351,26 @@ const DEFAULT_CONFIG: HyperframesConfig = { let cachedConfig: HyperframesConfig | null = null; +/** A non-empty string, or undefined — hand-edited configs can carry anything. */ +function parseNonEmptyString(value: unknown): string | undefined { + return typeof value === "string" && value.length > 0 ? value : undefined; +} + +/** Shape-validate the recent-renders ring from a parsed config. */ +function parseRecentRenders(value: unknown): RecentRenderRecord[] | undefined { + if (!Array.isArray(value)) return undefined; + return value + .filter( + (r): r is RecentRenderRecord => + typeof r === "object" && + r !== null && + typeof (r as RecentRenderRecord).id === "string" && + typeof (r as RecentRenderRecord).at === "string" && + typeof (r as RecentRenderRecord).ok === "boolean", + ) + .slice(-MAX_RECENT_RENDERS); +} + /** * Read the config file, creating it with defaults if it doesn't exist. * Returns a mutable copy — call `writeConfig()` to persist changes. @@ -356,20 +417,23 @@ export function readConfig(): HyperframesConfig { : undefined, predecessorFound: typeof parsed.predecessorFound === "boolean" ? parsed.predecessorFound : undefined, - recentRenders: Array.isArray(parsed.recentRenders) - ? parsed.recentRenders - .filter( - (r): r is RecentRenderRecord => - typeof r === "object" && - r !== null && - typeof (r as RecentRenderRecord).id === "string" && - typeof (r as RecentRenderRecord).at === "string" && - typeof (r as RecentRenderRecord).ok === "boolean", - ) - .slice(-MAX_RECENT_RENDERS) - : undefined, + bucketSeed: parseNonEmptyString(parsed.bucketSeed), + recentRenders: parseRecentRenders(parsed.recentRenders), }; + // One-time backfill for configs predating the bucket seed: prefer the + // lineage seed if a previous install already recorded one, else mint. + // Persisted immediately — an unpersisted seed would re-roll every process. + if (config.bucketSeed === undefined) { + config.bucketSeed = readInstallState()?.bucketSeed ?? randomUUID(); + writeConfig(config); + // Cache even if the write failed, so the seed is at least stable for + // the life of this process (a re-roll per readConfigFresh would flip + // cohorts mid-session). + cachedConfig = config; + return { ...config }; + } + cachedConfig = config; return { ...config }; } catch { diff --git a/packages/studio/src/telemetry/canary.test.ts b/packages/studio/src/telemetry/canary.test.ts index 5ae4077215..bd51b998b8 100644 --- a/packages/studio/src/telemetry/canary.test.ts +++ b/packages/studio/src/telemetry/canary.test.ts @@ -47,6 +47,7 @@ beforeEach(() => { sessionStorage.clear(); setSearch(""); delete window.__HF_CLI_DISTINCT_ID; + delete window.__HF_CLI_BUCKET_SEED; Object.defineProperty(navigator, "webdriver", { value: false, configurable: true }); __resetStudioCanaryCacheForTests(); __resetStudioDistinctIdForTests(); @@ -131,19 +132,35 @@ describe("automated browsers", () => { }); describe("cohort identity", () => { - it("buckets on the Studio distinct id unmodified — so a CLI-launched Studio shares the CLI's cohort", () => { - // distinctId.ts adopts window.__HF_CLI_DISTINCT_ID when the CLI launched - // Studio. This asserts the binding passes that id through untouched: if it - // prefixed or re-hashed it, the editor would land in a different cohort - // than the terminal for the same user, and a rollout spanning both would - // be incoherent. + it("buckets on the CLI's bucket seed when injected — the unit that survives config wipes", () => { + // The CLI buckets on its bucketSeed (inherited across config wipes via + // the install-state file), so a CLI-launched Studio must bucket on the + // SAME seed or the two surfaces would split one machine across cohorts. + const cliId = "db0c1f4a-b95e-4c35-90c6-1a15bd76f717"; + const cliSeed = "5f1c9d2e-0000-4000-8000-aaaaaaaaaaaa"; + window.__HF_CLI_DISTINCT_ID = cliId; + window.__HF_CLI_BUCKET_SEED = cliSeed; + __resetStudioDistinctIdForTests(); + __resetStudioCanaryCacheForTests(); + + // Telemetry identity still adopts the DISTINCT id — the seed only buckets. + expect(resolveStudioDistinctId()).toBe(cliId); + const viaBinding = resolveCanary("on-everywhere").bucket; + const bySeed = evaluateCanary({ + feature: "on-everywhere", + unitId: cliSeed, + percentage: 100, + }).bucket; + expect(viaBinding).toBe(bySeed); + }); + + it("buckets on the Studio distinct id when no seed is injected (standalone Studio)", () => { const cliId = "db0c1f4a-b95e-4c35-90c6-1a15bd76f717"; window.__HF_CLI_DISTINCT_ID = cliId; __resetStudioDistinctIdForTests(); __resetStudioCanaryCacheForTests(); expect(resolveStudioDistinctId()).toBe(cliId); - // 50% so the answer is id-dependent rather than trivially true. const viaBinding = resolveCanary("on-everywhere").bucket; const direct = evaluateCanary({ feature: "on-everywhere", diff --git a/packages/studio/src/telemetry/canary.ts b/packages/studio/src/telemetry/canary.ts index bf8b13905b..af95db2b81 100644 --- a/packages/studio/src/telemetry/canary.ts +++ b/packages/studio/src/telemetry/canary.ts @@ -14,11 +14,11 @@ // // Three things differ from the CLI, each for a reason: // -// 1. UNIT ID — `resolveStudioDistinctId()` instead of the CLI's config file. -// That function already adopts `window.__HF_CLI_DISTINCT_ID` when the CLI -// launched Studio, so a CLI-launched Studio lands in the SAME cohort as the -// CLI itself: a rollout spanning render and editor is coherent for that -// user instead of enrolling their terminal but not their editor. +// 1. UNIT ID — the CLI's bucket seed (`window.__HF_CLI_BUCKET_SEED`) when the +// CLI launched Studio, so both surfaces bucket on the SAME unit and a +// rollout spanning render and editor is coherent for that user. Standalone +// Studio falls back to `resolveStudioDistinctId()` — the browser has no +// second storage location, so its localStorage id doubles as the seed. // // 2. OVERRIDE — there is no `process.env` in a page, so the override is a URL // query param mirrored into sessionStorage (see `readOverride`). @@ -48,6 +48,30 @@ export function canaryParamName(name: string): string { return `hf_canary_${name.toLowerCase().replace(/[^a-z0-9]+/g, "_")}`; } +// Injected by the CLI's studio server alongside __HF_CLI_DISTINCT_ID. +declare global { + interface Window { + __HF_CLI_BUCKET_SEED?: string; + } +} + +/** + * The bucketing unit. A CLI-launched Studio buckets on the CLI's SEED (which + * survives config wipes via the install-state file), not its distinct id — + * the CLI itself buckets on the seed, and the two surfaces must agree per + * machine. Standalone Studio falls back to its own distinct id: the browser + * has no second storage location, so localStorage IS both id and seed there. + */ +function resolveBucketUnit(): string { + try { + const seed = typeof window === "undefined" ? undefined : window.__HF_CLI_BUCKET_SEED; + if (typeof seed === "string" && seed.length > 0) return seed; + } catch { + /* fall through */ + } + return resolveStudioDistinctId(); +} + const STORAGE_PREFIX = "hyperframes-studio:canary:"; /** @@ -130,7 +154,7 @@ export function resolveCanary(name: string): CanaryDecision { const decision: CanaryDecision = definition ? evaluateCanary({ feature: definition.name, - unitId: resolveStudioDistinctId(), + unitId: resolveBucketUnit(), percentage: definition.percentage, override: readOverride(definition.name), exclude: isAutomatedBrowser(), From 9f2b892a7137dc14b3e58a0773ba55384562d875 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 30 Jul 2026 15:21:46 -0700 Subject: [PATCH 13/20] fix(cli): keep the canary bucket seed inside the config dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopts the #2904 pattern for bucketSeed. The seed rides in install-state.json, which now lives beside config.json in ~/.hyperframes rather than in ~/.local/state/hyperframes/. Review rejected persisting state outside the config dir to defeat a user's reset, and that objection is sharpest for the seed: it is the one field that would turn install-state into a persistent pseudonymous identifier surviving `rm -rf ~/.hyperframes`. The carryover still earns its place, just against the churn that actually happens. config.json is rewritten on every command and every render, and readConfig recovers from any parse/permission/IO failure by minting a fresh identity — so a re-mint would reshuffle cohorts mid-rollout. A no-schema file written once at mint is decoupled from that without leaving the directory. Config re-mint: cohorts hold. Directory deleted: cohorts go too, deliberately. A pre-move seed is adopted by the same migration, so installs already carrying one do not have a live cohort reshuffled under them. Tests: seed survives a re-mint, does NOT survive deleting the config dir, and migrates from the pre-move path. Prose in config.ts, canary.ts and canary-rollouts.mdx corrected — it still claimed cohorts survive a wipe. Docs gain a removal-path section. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 52 +++++++++++++++-------- packages/cli/src/telemetry/canary.ts | 7 +-- packages/cli/src/telemetry/config.test.ts | 34 ++++++++++++--- packages/cli/src/telemetry/config.ts | 44 ++++++++++++------- 4 files changed, 96 insertions(+), 41 deletions(-) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index 4b7344dac0..0124f89a19 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -167,24 +167,30 @@ experiment, and it survives uninstall), and account identity covers only accepted limit — but its worst consequence is mitigated, and its size is now directly measurable rather than inferred: -- **Cohorts survive the wipe.** Canaries bucket on a dedicated `bucketSeed` — - not the telemetry id — and the seed is inherited across config wipes via a - machine-local state file (write-once: the first install's seed is the - lineage's seed forever). A wipe re-rolls the telemetry id, never the canary - assignment. The seed is never emitted, so it does not link the old id to - the new one. -- **A re-rolled install cannot re-enter a path that already failed on that +- **Cohorts survive a config re-mint.** Canaries bucket on a dedicated + `bucketSeed` — not the telemetry id — held in `install-state.json` beside + `config.json` and write-once (the first install's seed wins forever). This + matters because `config.json` is rewritten on every command and every + render, and any parse/permission/IO failure makes the CLI mint a fresh + identity. The seed is never emitted, so it does not link the old id to the + new one. +- **Deleting `~/.hyperframes` clears the cohort too, deliberately.** The seed + does not live outside the config directory, so the user's reset is a real + reset. A seed that outlived it would be a persistent identifier defeating + the only lever they have — see [the removal path](#removing-your-canary-state). +- **A re-minted install cannot re-enter a path that already failed on that machine.** The circuit breaker's tripped state is mirrored to the same state file. - **`install_predecessor_found`** on every event says whether this install's - mint found a previous install's state marker. Its true-share splits check - 2's drift into the recoverable part (config wiped, machine persisted) and - the part no local mechanism can link (fresh machine, container, new user). + mint found a previous install's state marker — i.e. `config.json` was lost + while `install-state.json` survived. Its true-share is the re-mint rate + directly; drift from a deliberate reset, a fresh machine, a container, or a + new user is not linkable by any local mechanism and is not counted. With the seed carryover, check 2's residual drift comes from the -unrecoverable buckets only — fresh machines, containers, and genuinely new -users — and canary window lengths get picked from that residual, not the raw -turnover. +unrecoverable buckets only — deliberate resets, fresh machines, containers, +and genuinely new users — and canary window lengths get picked from that +residual, not the raw turnover. ## Behaviour worth knowing @@ -197,10 +203,22 @@ not the seed alone — two canaries at 10% select two different 10%s. If they shared a slice, one unlucky cohort would receive every experiment at once and no two rollouts could be read apart. -**Cohorts are keyed to the machine, not the config.** The bucketing unit is a -dedicated seed inherited across config wipes (see the calibration section) — -distinct from the telemetry id, never emitted, and shared with a CLI-launched -Studio so both surfaces agree. +**Cohorts are keyed to the install directory, not to `config.json`.** The +bucketing unit is a dedicated seed in `install-state.json`, inherited across +`config.json` re-mints (see the calibration section) — distinct from the +telemetry id, never emitted, and shared with a CLI-launched Studio so both +surfaces agree. It does not outlive `~/.hyperframes`. + +## Removing your canary state + +Both `config.json` and `install-state.json` live in `~/.hyperframes`, so: + +```bash +rm -rf ~/.hyperframes # clears telemetry id, canary cohorts, and breaker state +``` + +`hyperframes telemetry status` prints both paths if you want to inspect or +delete them individually. Nothing canary-related is stored anywhere else. **It fails closed.** No install id, an unregistered name, or a CI machine all resolve to *not enrolled*. A canary exists to bound blast radius, so "we diff --git a/packages/cli/src/telemetry/canary.ts b/packages/cli/src/telemetry/canary.ts index 49d86918ef..f1d0d95c5a 100644 --- a/packages/cli/src/telemetry/canary.ts +++ b/packages/cli/src/telemetry/canary.ts @@ -60,9 +60,10 @@ export function resolveCanary(name: string): CanaryDecision { ? evaluateCanary({ feature: definition.name, // The bucket seed, NOT the anonymousId: the seed is inherited across - // config wipes via the install-state file, so the machine keeps its - // cohorts when the telemetry id re-rolls. Fallback covers only a - // failed backfill write on a legacy config. + // config.json re-mints via the install-state file, so cohorts hold + // when the telemetry id re-rolls. Both files live in ~/.hyperframes, + // so deleting that directory clears the cohort too — intentional. + // Fallback covers only a failed backfill write on a legacy config. unitId: config.bucketSeed ?? config.anonymousId, percentage: definition.percentage, override: parseCanaryOverride(process.env[canaryEnvVar(definition.name)]), diff --git a/packages/cli/src/telemetry/config.test.ts b/packages/cli/src/telemetry/config.test.ts index 646284eda6..2881f9778c 100644 --- a/packages/cli/src/telemetry/config.test.ts +++ b/packages/cli/src/telemetry/config.test.ts @@ -36,6 +36,11 @@ vi.mock("node:fs", () => ({ }), })); +// 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. +const LEGACY_STATE_PATH = join(homedir(), ".local", "state", "hyperframes", "install-state.json"); + describe("config.ts — readConfig / readConfigFresh / writeConfig (real module, faked fs)", () => { let readConfig: typeof import("./config.js").readConfig; let readConfigFresh: typeof import("./config.js").readConfigFresh; @@ -143,11 +148,6 @@ describe("install-state rollover (breaker survives a config re-mint)", () => { let CONFIG_PATH: typeof import("./config.js").CONFIG_PATH; let STATE_PATH: typeof import("./config.js").STATE_PATH; - // 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. - const LEGACY_STATE_PATH = join(homedir(), ".local", "state", "hyperframes", "install-state.json"); - beforeEach(async () => { fsState.files.clear(); vi.resetModules(); @@ -322,7 +322,7 @@ describe("bucket-seed carryover (cohorts survive a config wipe)", () => { expect(state.bucketSeed).toBe(config.bucketSeed); }); - it("the seed survives a config wipe — the machine keeps its cohorts, only the id re-rolls", () => { + it("the seed survives a config.json re-mint — cohorts hold, only the id re-rolls", () => { const first = readConfig(); fsState.files.delete(CONFIG_PATH); const second = readConfigFresh(); @@ -330,6 +330,28 @@ describe("bucket-seed carryover (cohorts survive a config wipe)", () => { expect(second.anonymousId).not.toBe(first.anonymousId); }); + // The counterpart, and the reason install-state shares CONFIG_DIR: a seed + // that outlived `rm -rf ~/.hyperframes` would be a persistent identifier + // defeating the only reset the user has. + it("the seed does NOT survive deleting the config dir — that reset is real", () => { + const first = readConfig(); + fsState.files.delete(CONFIG_PATH); + fsState.files.delete(STATE_PATH); + const reborn = readConfigFresh(); + expect(reborn.bucketSeed).toBeTruthy(); + expect(reborn.bucketSeed).not.toBe(first.bucketSeed); + expect(reborn.predecessorFound).toBe(false); + }); + + it("adopts a pre-move seed so the migration does not reshuffle a live cohort", () => { + fsState.files.set( + LEGACY_STATE_PATH, + JSON.stringify({ markerAt: "2026-07-28T00:00:00.000Z", bucketSeed: "seed-from-old-path" }), + ); + expect(readConfig().bucketSeed).toBe("seed-from-old-path"); + expect(fsState.files.has(LEGACY_STATE_PATH), "legacy copy removed").toBe(false); + }); + it("the seed survives config corruption via the same mint path", () => { const first = readConfig(); fsState.files.set(CONFIG_PATH, "{not valid json"); diff --git a/packages/cli/src/telemetry/config.ts b/packages/cli/src/telemetry/config.ts index 23c254268e..6482c563dc 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -29,8 +29,8 @@ const CONFIG_FILE = join(CONFIG_DIR, "config.json"); // that churn: no shared schema to migrate on upgrade, and one write at first // mint instead of one per command. // -// It carries exactly two facts, and no identity — no anonymousId, no -// counters, nothing linking the old install to the new one: +// It carries exactly three facts, and no telemetry identity — no anonymousId, +// no counters, nothing emitted that links the old install to the new one: // // 1. `markerAt` — "a hyperframes install existed on this machine". Now that // it shares CONFIG_DIR, `predecessorFound` measures the churn we care @@ -39,6 +39,10 @@ const CONFIG_FILE = join(CONFIG_DIR, "config.json"); // 2. `deParallelRouterTrialFired` — the DE parallel-router circuit // breaker's tripped state, so a config re-mint does not re-enrol an // install into an experimental path that already FAILED on this machine. +// 3. `bucketSeed` — the canary bucketing unit, so a re-mint does not +// reshuffle cohorts mid-rollout. Never transmitted; only the resulting +// true/false assignments are. Sharing CONFIG_DIR is what keeps it from +// being a persistent identifier that outlives the user's reset. // // Removal path: delete ~/.hyperframes (or just this file). `hyperframes // telemetry status` prints its exact location. @@ -57,9 +61,10 @@ interface InstallState { /** Rolled-over circuit-breaker state — see HyperframesConfig's field. */ deParallelRouterTrialFired?: boolean; /** - * The machine's canary bucketing seed — see HyperframesConfig's field. - * Write-once: the first install's seed is the lineage's seed forever, so a - * config wipe re-mints the telemetry id but NOT the canary cohorts. + * The canary bucketing seed — see HyperframesConfig's field. Write-once + * within this config dir: the first install's seed wins forever, so a + * config.json re-mint re-rolls the telemetry id but NOT the canary cohorts. + * Deleting ~/.hyperframes takes the seed with it, by design. */ bucketSeed?: string; } @@ -152,7 +157,7 @@ function sameInstallState(a: InstallState, b: InstallState): boolean { ); } -/** Latching: once tripped (by any install in this machine's lineage), stays tripped. */ +/** Latching: once tripped (by any install in this config dir), stays tripped. */ function latchedFired(state: InstallState | null, config: HyperframesConfig): true | undefined { return ( state?.deParallelRouterTrialFired === true || @@ -169,7 +174,7 @@ function nextInstallState( const next: InstallState = { markerAt: state?.markerAt ?? new Date().toISOString(), deParallelRouterTrialFired: latchedFired(state, config), - // Write-once: an existing lineage seed always wins, so cohorts stay + // Write-once: an existing seed always wins, so cohorts stay // anchored to the FIRST install in this config dir, not the latest one. bucketSeed: state?.bucketSeed ?? config.bucketSeed, }; @@ -289,7 +294,8 @@ export interface HyperframesConfig { /** * Whether a previous install's state marker existed on this machine when * this config was minted. Attached to telemetry so the fraction of fresh - * installs that are RECOVERABLE churn (config wiped, machine persisted) is + * installs that are RECOVERABLE churn (config.json re-minted, install-state + * survived) is * measurable directly. `undefined` on configs minted before this field * existed — a different fact from `false` (minted fresh, no predecessor). */ @@ -297,12 +303,20 @@ export interface HyperframesConfig { /** * The unit canary percentages bucket on — deliberately NOT the anonymousId. * A fresh random UUID, mirrored write-once into the install-state file and - * inherited at mint, so a config wipe re-rolls the telemetry id but keeps - * the machine's canary cohorts: no cumulative-exposure drift from wipes, - * and before/after comparisons survive a reinstall. It is never emitted in - * telemetry (only the resulting true/false assignments are), so it does not - * link the old id to the new one server-side. Backfilled once for configs - * predating the field. + * inherited at mint, so a config.json RE-MINT re-rolls the telemetry id but + * keeps this install's canary cohorts: no cumulative-exposure drift from + * corruption, and before/after comparisons survive one. + * + * A re-mint is the churn that actually happens — config.json is rewritten on + * every command and every render, and readConfig recovers from any + * parse/permission/IO failure by minting fresh. Deleting ~/.hyperframes + * deliberately does NOT survive: install-state lives in that same directory + * precisely so the user's reset is a real reset. A seed that outlived it + * would be a persistent identifier defeating the only lever they have. + * + * Never emitted in telemetry (only the resulting true/false assignments + * are), so it does not link the old id to the new one server-side. + * Backfilled once for configs predating the field. */ bucketSeed?: string; /** @@ -422,7 +436,7 @@ export function readConfig(): HyperframesConfig { }; // One-time backfill for configs predating the bucket seed: prefer the - // lineage seed if a previous install already recorded one, else mint. + // recorded seed if a previous install already wrote one, else mint. // Persisted immediately — an unpersisted seed would re-roll every process. if (config.bucketSeed === undefined) { config.bucketSeed = readInstallState()?.bucketSeed ?? randomUUID(); From 4f464dc424c648c1210fbfd0675ff774ee0037d4 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 30 Jul 2026 16:11:34 -0700 Subject: [PATCH 14/20] feat(cli,studio): telemetry opt-out is canary opt-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers flagged the same gap: seed injection was gated on telemetryShouldTrack(), but canary EVALUATION was not. An install with DO_NOT_TRACK=1 was still bucketed and still had real code paths flipped (e.g. HF_DE_PARALLEL_ROUTER), silently and unmeasurably. A canary is a measured rollout — we enrol a slice precisely so it can be compared against everyone else. An install that sends nothing can't be compared, so enrolling it buys no signal and only changes that user's code path, on an experimental feature, without their knowledge. That is the wrong side of an opt-out. Resolves to a new `telemetry_opt_out` reason BEFORE bucketing, so no cohort is assigned at all. Distinct from `excluded` because "why is my canary off" has a very different answer for CI than for opted-out, and the reason never reaches telemetry by construction. Covers every opt-out route: persisted preference, the runtime env vars and dev/telemetry-disabled builds via policy.ts, and Studio's hyperframes-studio:telemetryDisabled. An explicit HF_CANARY_* / ?hf_canary_*= override still wins — a deliberate local choice, not silent enrolment, and the documented way to exercise a canary with telemetry off. The CLI check mirrors shouldTrack() rather than importing it: client.ts already imports canary.ts for canaryEventProperties, so depending on it would be a cycle. Both read the same two inputs, so they cannot disagree. Tests: 9 new across CLI and Studio (preference off, each runtime override, no bucket assigned, override still honoured, flag properties all-false). Fault injection: removing the CLI gate fails 6, removing the Studio gate fails 3. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 14 ++++ packages/cli/src/telemetry/canary.test.ts | 61 +++++++++++++++- packages/cli/src/telemetry/canary.ts | 76 ++++++++++++++------ packages/core/src/canary.ts | 8 ++- packages/studio/src/telemetry/canary.test.ts | 33 +++++++++ packages/studio/src/telemetry/canary.ts | 27 ++++--- 6 files changed, 186 insertions(+), 33 deletions(-) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index 0124f89a19..ea6f153863 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -220,6 +220,20 @@ rm -rf ~/.hyperframes # clears telemetry id, canary cohorts, and breaker stat `hyperframes telemetry status` prints both paths if you want to inspect or delete them individually. Nothing canary-related is stored anywhere else. +**Opting out of telemetry opts you out of canaries.** A canary is a *measured* +rollout — we enrol a slice precisely so it can be compared against everyone +else. An install that sends nothing can't be compared, so enrolling it buys no +signal and only changes that user's code path, on an experimental feature, +without their knowledge. Every opt-out route counts: the persisted preference +(`hyperframes telemetry disable`), the runtime env vars +(`HYPERFRAMES_NO_TELEMETRY`, `DO_NOT_TRACK`), dev/telemetry-disabled builds, +and Studio's `hyperframes-studio:telemetryDisabled`. The decision resolves to +`telemetry_opt_out` *before* bucketing, so no cohort is assigned at all. + +An explicit `HF_CANARY_` (or `?hf_canary_=` in Studio) +override still wins — that is a deliberate local choice, and it stays the way +to exercise a canary with telemetry off. + **It fails closed.** No install id, an unregistered name, or a CI machine all resolve to *not enrolled*. A canary exists to bound blast radius, so "we don't know who this is" must never mean "enrol everyone". CI is excluded diff --git a/packages/cli/src/telemetry/canary.test.ts b/packages/cli/src/telemetry/canary.test.ts index f1d8b8b10a..80582db91f 100644 --- a/packages/cli/src/telemetry/canary.test.ts +++ b/packages/cli/src/telemetry/canary.test.ts @@ -1,17 +1,32 @@ import { describe, expect, it, vi, beforeEach } from "vitest"; -const configState: { anonymousId: string; bucketSeed: string | undefined } = { +const configState: { + anonymousId: string; + bucketSeed: string | undefined; + telemetryEnabled: boolean; +} = { anonymousId: "db0c1f4a-b95e-4c35-90c6-1a15bd76f717", bucketSeed: undefined, + telemetryEnabled: true, }; const systemState = { is_ci: false }; +// null = no runtime opt-out in force; a string names the source (env var, +// dev build, ...) exactly as policy.ts reports it. +const policyState: { runtimeOverride: string | null } = { runtimeOverride: null }; vi.mock("./config.js", () => ({ - readConfig: () => ({ anonymousId: configState.anonymousId, bucketSeed: configState.bucketSeed }), + readConfig: () => ({ + anonymousId: configState.anonymousId, + bucketSeed: configState.bucketSeed, + telemetryEnabled: configState.telemetryEnabled, + }), })); vi.mock("./system.js", () => ({ getSystemMeta: () => ({ is_ci: systemState.is_ci }), })); +vi.mock("./policy.js", () => ({ + telemetryRuntimeOverride: () => policyState.runtimeOverride, +})); // The registry is data; pin a known shape so these tests don't move when a // real canary is added or ramped. @@ -64,11 +79,53 @@ beforeEach(() => { __resetCanaryCacheForTests(); configState.anonymousId = "db0c1f4a-b95e-4c35-90c6-1a15bd76f717"; configState.bucketSeed = undefined; + configState.telemetryEnabled = true; systemState.is_ci = false; + policyState.runtimeOverride = null; delete process.env.HF_CANARY_TEST_ALPHA; delete process.env.HF_CANARY_TEST_BETA; }); +describe("telemetry opt-out is canary opt-out", () => { + it("does not enrol when the persisted preference is off", () => { + configState.telemetryEnabled = false; + // test-alpha is at 100% — it would be on for everyone otherwise. + expect(resolveCanary("test-alpha")).toEqual({ + enabled: false, + reason: "telemetry_opt_out", + }); + }); + + it.each(["HYPERFRAMES_NO_TELEMETRY", "DO_NOT_TRACK", "dev_mode"])( + "does not enrol under the %s runtime override, even with the preference on", + (source) => { + configState.telemetryEnabled = true; + policyState.runtimeOverride = source; + expect(resolveCanary("test-alpha").reason).toBe("telemetry_opt_out"); + }, + ); + + it("never buckets an opted-out install — no cohort is assigned at all", () => { + configState.telemetryEnabled = false; + // A bucket number would mean we hashed them into a slice anyway. + expect(resolveCanary("test-alpha").bucket).toBeUndefined(); + }); + + it("still honours an explicit override — the documented way to test with telemetry off", () => { + configState.telemetryEnabled = false; + process.env.HF_CANARY_TEST_BETA = "on"; + expect(resolveCanary("test-beta")).toEqual({ enabled: true, reason: "forced_on" }); + }); + + it("reports every canary as false to PostHog shape when opted out", () => { + configState.telemetryEnabled = false; + expect(canaryEventProperties()).toEqual({ + "$feature/canary-test-alpha": "false", + "$feature/canary-test-beta": "false", + }); + }); +}); + describe("bucketing unit", () => { it("buckets on the bucketSeed when present — the unit that survives config wipes", async () => { const { evaluateCanary } = await import("@hyperframes/core/canary"); diff --git a/packages/cli/src/telemetry/canary.ts b/packages/cli/src/telemetry/canary.ts index f1d0d95c5a..d76708d80d 100644 --- a/packages/cli/src/telemetry/canary.ts +++ b/packages/cli/src/telemetry/canary.ts @@ -28,8 +28,28 @@ import { } from "@hyperframes/core/canary"; import { CANARIES, canaryEnvVar, findCanary } from "@hyperframes/core/canary-registry"; import { readConfig } from "./config.js"; +import { telemetryRuntimeOverride } from "./policy.js"; import { getSystemMeta } from "./system.js"; +/** + * Opting out of telemetry opts you out of canaries. + * + * A canary is a measured rollout: we enrol a slice precisely so we can compare + * it against everyone else. An install that sends nothing can't be compared, + * so enrolling it buys no signal — it only changes that user's code path, on + * an experimental feature, without their knowledge and with no way for us to + * see the result. That is the wrong side of an opt-out. + * + * Deliberately mirrors `shouldTrack()` rather than importing it: client.ts + * already imports this module for `canaryEventProperties`, so depending on it + * here would be a cycle. Both read the same two inputs (`policy.ts`'s runtime + * override, then the persisted preference), so they cannot disagree. + */ +function telemetryActive(): boolean { + if (telemetryRuntimeOverride() !== null) return false; + return readConfig().telemetryEnabled; +} + /** * Decisions are memoized per process: a `--batch` run asks the same question * once per row, and a canary must not change its mind mid-process — a render @@ -43,6 +63,40 @@ export function __resetCanaryCacheForTests(): void { decisions.clear(); } +/** The uncached decision. Split out so `resolveCanary` is purely the memo. */ +function decideCanary(name: string): CanaryDecision { + const definition = findCanary(name); + if (!definition) return { enabled: false, reason: "out_of_cohort" }; + + const override = parseCanaryOverride(process.env[canaryEnvVar(definition.name)]); + + // Resolved ahead of evaluate so an opted-out install is never bucketed at + // all. An explicit HF_CANARY_* override still wins: that is a deliberate + // local choice (support session, bisect, developer testing), not silent + // enrolment, and it stays the way to exercise a canary with telemetry off. + if (override === undefined && !telemetryActive()) { + return { enabled: false, reason: "telemetry_opt_out" }; + } + + const config = readConfig(); + return evaluateCanary({ + feature: definition.name, + // The bucket seed, NOT the anonymousId: the seed is inherited across + // config.json re-mints via the install-state file, so cohorts hold when + // the telemetry id re-rolls. Both files live in ~/.hyperframes, so + // deleting that directory clears the cohort too — intentional. Fallback + // covers only a failed backfill write on a legacy config. + unitId: config.bucketSeed ?? config.anonymousId, + percentage: definition.percentage, + override, + // CI installs regenerate their config per run, so their ids are + // ephemeral — they would hop cohorts between runs, adding noise to the + // rollout signal while saying nothing about real users. An explicit + // override still gets through, which is how you test a canary in CI. + exclude: getSystemMeta().is_ci, + }); +} + /** * Full decision for a registered canary, including the reason — use this when * you want to record WHY, not just whether. @@ -54,27 +108,7 @@ export function resolveCanary(name: string): CanaryDecision { const cached = decisions.get(name); if (cached) return cached; - const definition = findCanary(name); - const config = readConfig(); - const decision: CanaryDecision = definition - ? evaluateCanary({ - feature: definition.name, - // The bucket seed, NOT the anonymousId: the seed is inherited across - // config.json re-mints via the install-state file, so cohorts hold - // when the telemetry id re-rolls. Both files live in ~/.hyperframes, - // so deleting that directory clears the cohort too — intentional. - // Fallback covers only a failed backfill write on a legacy config. - unitId: config.bucketSeed ?? config.anonymousId, - percentage: definition.percentage, - override: parseCanaryOverride(process.env[canaryEnvVar(definition.name)]), - // CI installs regenerate their config per run, so their ids are - // ephemeral — they would hop cohorts between runs, adding noise to the - // rollout signal while saying nothing about real users. An explicit - // override still gets through, which is how you test a canary in CI. - exclude: getSystemMeta().is_ci, - }) - : { enabled: false, reason: "out_of_cohort" }; - + const decision = decideCanary(name); decisions.set(name, decision); return decision; } diff --git a/packages/core/src/canary.ts b/packages/core/src/canary.ts index e966423386..10dd4fa123 100644 --- a/packages/core/src/canary.ts +++ b/packages/core/src/canary.ts @@ -39,7 +39,13 @@ export type CanaryReason = | "in_cohort" | "out_of_cohort" | "no_unit_id" - | "excluded"; + | "excluded" + // Telemetry is off, so the install is not enrolled. Distinct from + // "excluded" because the caller decides this BEFORE evaluate is reached — + // and because "why is my canary off" has a very different answer in the two + // cases. Never appears in telemetry by construction: an install that + // resolves this way sends nothing. + | "telemetry_opt_out"; export interface CanaryDecision { enabled: boolean; diff --git a/packages/studio/src/telemetry/canary.test.ts b/packages/studio/src/telemetry/canary.test.ts index bd51b998b8..ff89f6a973 100644 --- a/packages/studio/src/telemetry/canary.test.ts +++ b/packages/studio/src/telemetry/canary.test.ts @@ -192,3 +192,36 @@ describe("telemetry", () => { expect(canaryEventProperties()["$feature/canary-on-everywhere"]).toBe("false"); }); }); + +describe("telemetry opt-out is canary opt-out", () => { + // The studio opt-out lever, per telemetry/config.ts. + const OPT_OUT_KEY = "hyperframes-studio:telemetryDisabled"; + + it("does not enrol an opted-out browser profile", () => { + localStorage.setItem(OPT_OUT_KEY, "1"); + // on-everywhere is at 100% — it would be on for everyone otherwise. + expect(resolveCanary("on-everywhere")).toEqual({ + enabled: false, + reason: "telemetry_opt_out", + }); + }); + + it("never buckets an opted-out profile — no cohort is assigned at all", () => { + localStorage.setItem(OPT_OUT_KEY, "1"); + expect(resolveCanary("on-everywhere").bucket).toBeUndefined(); + }); + + it("still honours an explicit URL override", () => { + localStorage.setItem(OPT_OUT_KEY, "1"); + setSearch("?hf_canary_off_everywhere=on"); + expect(resolveCanary("off-everywhere")).toEqual({ enabled: true, reason: "forced_on" }); + }); + + it("reports every canary as false when opted out", () => { + localStorage.setItem(OPT_OUT_KEY, "1"); + expect(canaryEventProperties()).toEqual({ + "$feature/canary-on-everywhere": "false", + "$feature/canary-off-everywhere": "false", + }); + }); +}); diff --git a/packages/studio/src/telemetry/canary.ts b/packages/studio/src/telemetry/canary.ts index af95db2b81..943ed720f3 100644 --- a/packages/studio/src/telemetry/canary.ts +++ b/packages/studio/src/telemetry/canary.ts @@ -41,6 +41,7 @@ import { } from "@hyperframes/core/canary"; import { CANARIES, findCanary } from "@hyperframes/core/canary-registry"; import { resolveStudioDistinctId } from "./distinctId"; +import { isOptedOut } from "./config"; import { safeSessionStorage } from "../utils/safeStorage"; /** `my-feature` → `hf_canary_my_feature`, the query param and storage key. */ @@ -151,15 +152,23 @@ export function resolveCanary(name: string): CanaryDecision { if (cached) return cached; const definition = findCanary(name); - const decision: CanaryDecision = definition - ? evaluateCanary({ - feature: definition.name, - unitId: resolveBucketUnit(), - percentage: definition.percentage, - override: readOverride(definition.name), - exclude: isAutomatedBrowser(), - }) - : { enabled: false, reason: "out_of_cohort" }; + const override = definition ? readOverride(definition.name) : undefined; + // Opting out of telemetry opts you out of canaries — same rule as the CLI + // (see packages/cli/src/telemetry/canary.ts). An install that sends nothing + // can't be compared against anyone, so enrolling it changes that user's + // code path for no signal. An explicit override still wins: that is a + // deliberate local choice, not silent enrolment. + const decision: CanaryDecision = !definition + ? { enabled: false, reason: "out_of_cohort" } + : override === undefined && isOptedOut() + ? { enabled: false, reason: "telemetry_opt_out" } + : evaluateCanary({ + feature: definition.name, + unitId: resolveBucketUnit(), + percentage: definition.percentage, + override, + exclude: isAutomatedBrowser(), + }); decisions.set(name, decision); return decision; From 98b23a8850dfe01a9f12957eef25a240294cbe21 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 30 Jul 2026 16:29:31 -0700 Subject: [PATCH 15/20] fix(cli,studio): adopt the CLI's canary decisions in a launched Studio MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes both cross-surface findings with one mechanism. The CLI publishes window.__HF_CLI_CANARY_DECISIONS ({ name: boolean }); a CLI-launched Studio takes it as authoritative over its own seed, URL override and the registry percentage. Studio re-deriving could not agree with the CLI in three cases: - Telemetry off. The CLI resolves telemetry_opt_out, but Studio's opt-out is a separate localStorage flag it cannot see, so it would evaluate normally and could enrol on a render the CLI excluded. The previous commit gated each surface independently; that fixed silent enrolment per surface but NOT the disagreement between them. - HF_CANARY_* override. Env vars never cross into the browser — Studio reads only its URL param / sessionStorage — so a support session forcing a canary on got the CLI forced and Studio guessing. - No seed injected. Studio falls back to a different unit id, i.e. a different bucket. Shipping the decision instead of the inputs makes divergence structurally impossible: one evaluation, two surfaces. It also exposes strictly less — booleans about features, rather than the seed buckets derive from — which is why it is safe to publish with telemetry off, the case it exists for. Studio still evaluates locally when standalone, or for a canary the CLI did not publish, and ignores a non-boolean value rather than trusting it. Tests: 6 Studio (CLI-off wins over unset local flag, CLI-on with no URL param, beats contradicting override, beats seed, falls back per-canary, rejects non-boolean) and 4 CLI (decisions with telemetry off and no identity, alongside identity when on, script-tag escaping on a hostile canary name, throwing resolver degrades to identity only). Four existing identity tests asserted the old "nothing when telemetry off" contract and were updated; the registry is now mocked there so string assertions don't move when a canary is added or ramped. Fault injection: dropping the adoption fails 4. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 18 ++++- .../cli/src/server/telemetryIdentity.test.ts | 61 +++++++++++++- packages/cli/src/server/telemetryIdentity.ts | 70 +++++++++++++--- packages/cli/src/telemetry/canary.ts | 26 ++++++ packages/studio/src/telemetry/canary.test.ts | 50 ++++++++++++ packages/studio/src/telemetry/canary.ts | 80 ++++++++++++++----- 6 files changed, 271 insertions(+), 34 deletions(-) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index ea6f153863..58981a4dbf 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -206,8 +206,22 @@ once and no two rollouts could be read apart. **Cohorts are keyed to the install directory, not to `config.json`.** The bucketing unit is a dedicated seed in `install-state.json`, inherited across `config.json` re-mints (see the calibration section) — distinct from the -telemetry id, never emitted, and shared with a CLI-launched Studio so both -surfaces agree. It does not outlive `~/.hyperframes`. +telemetry id and never emitted. It does not outlive `~/.hyperframes`. + +**A CLI-launched Studio adopts the CLI's decisions rather than re-deriving +them.** The CLI publishes `window.__HF_CLI_CANARY_DECISIONS` — a plain +`{ name: boolean }` map — and Studio takes it as authoritative over its own +seed, URL override and the registry percentage. Re-deriving cannot agree in +the cases that matter: telemetry off (the CLI resolves `telemetry_opt_out`, +but Studio's opt-out is a separate localStorage flag it cannot see), an +`HF_CANARY_*` override (env vars never cross into the browser), or no seed +injected (Studio falls back to a different unit, so a different bucket). One +render spanning both surfaces must not run half-enrolled. + +The decisions map is published even when telemetry is off — that is the case +it exists for. It is safe to expose where the seed is not: booleans about +features, not the value cohorts are derived from. Studio still evaluates +locally when opened standalone, or for any canary the CLI did not publish. ## Removing your canary state diff --git a/packages/cli/src/server/telemetryIdentity.test.ts b/packages/cli/src/server/telemetryIdentity.test.ts index cd5535bd27..2791a31698 100644 --- a/packages/cli/src/server/telemetryIdentity.test.ts +++ b/packages/cli/src/server/telemetryIdentity.test.ts @@ -6,6 +6,9 @@ import { describe, expect, it, vi, beforeEach } from "vitest"; const shouldTrack = vi.fn(); const readConfig = vi.fn(); +// Pinned rather than using the real registry, so these string assertions +// don't move every time a canary is added, ramped, or retired. +const canaryDecisions = vi.fn<() => Record>(); vi.mock("../telemetry/client.js", () => ({ shouldTrack: (...args: unknown[]) => shouldTrack(...args), @@ -13,6 +16,9 @@ vi.mock("../telemetry/client.js", () => ({ vi.mock("../telemetry/config.js", () => ({ readConfig: (...args: unknown[]) => readConfig(...args), })); +vi.mock("../telemetry/canary.js", () => ({ + canaryDecisionsForStudio: () => canaryDecisions(), +})); const { resolveCliTelemetryDistinctId, buildCliIdentityScript, buildStudioHeadScripts } = await import("./telemetryIdentity.js"); @@ -21,6 +27,8 @@ describe("resolveCliTelemetryDistinctId", () => { beforeEach(() => { shouldTrack.mockReset(); readConfig.mockReset(); + canaryDecisions.mockReset(); + canaryDecisions.mockReturnValue({}); }); it("returns the CLI anonymousId when telemetry is enabled", () => { @@ -56,6 +64,8 @@ describe("buildCliIdentityScript", () => { beforeEach(() => { shouldTrack.mockReset(); readConfig.mockReset(); + canaryDecisions.mockReset(); + canaryDecisions.mockReturnValue({}); }); it("emits a script that sets window.__HF_CLI_DISTINCT_ID when telemetry is on", () => { @@ -74,11 +84,56 @@ describe("buildCliIdentityScript", () => { ); }); - it("emits an empty string when telemetry is disabled (nothing to seed)", () => { + it("emits an empty string when telemetry is off and there are no canaries", () => { shouldTrack.mockReturnValue(false); expect(buildCliIdentityScript()).toBe(""); }); + // The cross-surface fix: with telemetry off the CLI resolves every canary + // to telemetry_opt_out, and Studio cannot see that from its own separate + // localStorage flag. Publishing the DECISIONS (not the identity) is what + // stops Studio evaluating independently and enrolling anyway. + it("still publishes canary decisions when telemetry is off, but no identity", () => { + shouldTrack.mockReturnValue(false); + canaryDecisions.mockReturnValue({ "de-parallel-router": false }); + const script = buildCliIdentityScript(); + expect(script).toBe( + '', + ); + expect(script).not.toContain("__HF_CLI_DISTINCT_ID"); + expect(script).not.toContain("__HF_CLI_BUCKET_SEED"); + }); + + it("publishes decisions alongside the identity when telemetry is on", () => { + shouldTrack.mockReturnValue(true); + readConfig.mockReturnValue({ anonymousId: "machine-uuid", bucketSeed: "seed-uuid" }); + canaryDecisions.mockReturnValue({ "de-parallel-router": true }); + expect(buildCliIdentityScript()).toBe( + '', + ); + }); + + it("escapes a canary name that tries to close the script tag", () => { + shouldTrack.mockReturnValue(false); + canaryDecisions.mockReturnValue({ "', + ); + }); + it("JSON-encodes the id so it can't break out of the script literal", () => { shouldTrack.mockReturnValue(true); readConfig.mockReturnValue({ anonymousId: ""; @@ -105,7 +162,7 @@ describe("buildStudioHeadScripts", () => { expect(head.indexOf("__HF_CLI_DISTINCT_ID")).toBeLessThan(head.indexOf("__HF_STUDIO_ENV__")); }); - it("returns just the env script when identity is suppressed (telemetry off)", () => { + it("returns just the env script when there is no identity and no canary", () => { shouldTrack.mockReturnValue(false); expect(buildStudioHeadScripts(ENV_SCRIPT)).toBe(ENV_SCRIPT); }); diff --git a/packages/cli/src/server/telemetryIdentity.ts b/packages/cli/src/server/telemetryIdentity.ts index 9a24765e4b..c9d843b1ed 100644 --- a/packages/cli/src/server/telemetryIdentity.ts +++ b/packages/cli/src/server/telemetryIdentity.ts @@ -18,6 +18,7 @@ import { readConfig } from "../telemetry/config.js"; import { shouldTrack as telemetryShouldTrack } from "../telemetry/client.js"; +import { canaryDecisionsForStudio } from "../telemetry/canary.js"; /** * The CLI's anonymous distinct id to hand to Studio, or null when CLI telemetry @@ -34,13 +35,6 @@ export function resolveCliTelemetryDistinctId(): string | null { } } -/** - * ``; + if (cliId) { + parts.push(`window.__HF_CLI_DISTINCT_ID=${encodeInlineScriptValue(cliId)};`); + const seed = resolveCliBucketSeed(); + if (seed) parts.push(`window.__HF_CLI_BUCKET_SEED=${encodeInlineScriptValue(seed)};`); + } + + // Emitted even when telemetry is OFF and the identity block above is empty — + // that is the case it exists for. With telemetry off the CLI resolves every + // canary to `telemetry_opt_out`, but Studio's opt-out is a separate + // localStorage flag it cannot see, so left to itself Studio would evaluate + // normally and could enrol on a render the CLI had already excluded. Same + // for an `HF_CANARY_*` override, which never crosses into the browser. + // + // Safe to publish unconditionally: these are booleans about features, not + // identity, and strictly less than the seed they replace as Studio's input. + const decisions = resolveCliCanaryDecisions(); + if (decisions !== null) { + parts.push(`window.__HF_CLI_CANARY_DECISIONS=${encodeInlineScriptJson(decisions)};`); + } + + return parts.length === 0 ? "" : ``; } /** diff --git a/packages/cli/src/telemetry/canary.ts b/packages/cli/src/telemetry/canary.ts index d76708d80d..f7684a7c54 100644 --- a/packages/cli/src/telemetry/canary.ts +++ b/packages/cli/src/telemetry/canary.ts @@ -118,6 +118,32 @@ export function isCanaryEnabled(name: string): boolean { return resolveCanary(name).enabled; } +/** + * Every registered canary's resolved on/off for this process, for handing to + * a CLI-launched Studio. + * + * Studio adopts these wholesale instead of re-deriving, because re-deriving + * cannot agree in three cases: + * + * - **Telemetry off.** The CLI resolves `telemetry_opt_out`; Studio's own + * opt-out is a separate localStorage flag on a different machine-level + * switch, so it would evaluate normally and could enrol. + * - **`HF_CANARY_*` override.** Env vars do not cross into the browser at + * all — Studio only reads its URL param / sessionStorage — so a support + * session forcing a canary on got the CLI forced and Studio guessing. + * - **No seed injected.** Whenever the seed is withheld Studio falls back + * to a different unit id, i.e. a different bucket. + * + * Shipping the decision rather than the inputs makes divergence structurally + * impossible: one evaluation, two surfaces. It is also strictly less to + * expose — booleans about features, not the seed the buckets derive from. + */ +export function canaryDecisionsForStudio(): Record { + const out: Record = {}; + for (const canary of CANARIES) out[canary.name] = resolveCanary(canary.name).enabled; + return out; +} + /** * Canary assignments as PostHog flag properties — `$feature/canary-` * set to `"true"` / `"false"` for every registered canary. Spread onto every diff --git a/packages/studio/src/telemetry/canary.test.ts b/packages/studio/src/telemetry/canary.test.ts index ff89f6a973..ab7b9d75cf 100644 --- a/packages/studio/src/telemetry/canary.test.ts +++ b/packages/studio/src/telemetry/canary.test.ts @@ -225,3 +225,53 @@ describe("telemetry opt-out is canary opt-out", () => { }); }); }); + +describe("CLI-launched Studio adopts the CLI's decisions", () => { + const OPT_OUT_KEY = "hyperframes-studio:telemetryDisabled"; + + afterEach(() => { + delete window.__HF_CLI_CANARY_DECISIONS; + }); + + // The divergence this exists for: CLI telemetry off resolves every canary + // to telemetry_opt_out, but Studio's opt-out is a SEPARATE localStorage + // flag it cannot see — left to itself it would evaluate and could enrol. + it("stays off when the CLI opted out, even though Studio's own flag is unset", () => { + expect(localStorage.getItem(OPT_OUT_KEY)).toBeNull(); + window.__HF_CLI_CANARY_DECISIONS = { "on-everywhere": false }; + expect(resolveCanary("on-everywhere").enabled).toBe(false); + }); + + // HF_CANARY_* never crosses into the browser, so before this the CLI was + // forced on and Studio silently guessed from the percentage. + it("turns on when the CLI forced it on, with no URL param present", () => { + window.__HF_CLI_CANARY_DECISIONS = { "off-everywhere": true }; + expect(resolveCanary("off-everywhere").enabled).toBe(true); + }); + + it("beats a contradicting URL override — one render must not run half-enrolled", () => { + window.__HF_CLI_CANARY_DECISIONS = { "on-everywhere": false }; + setSearch("?hf_canary_on_everywhere=on"); + expect(resolveCanary("on-everywhere").enabled).toBe(false); + }); + + it("beats the seed-derived bucket", () => { + window.__HF_CLI_BUCKET_SEED = "5f1c9d2e-0000-4000-8000-aaaaaaaaaaaa"; + window.__HF_CLI_CANARY_DECISIONS = { "on-everywhere": false }; + expect(resolveCanary("on-everywhere").enabled).toBe(false); + }); + + it("falls back to local evaluation for a canary the CLI did not publish", () => { + window.__HF_CLI_CANARY_DECISIONS = { "off-everywhere": true }; + expect(resolveCanary("on-everywhere").enabled).toBe(true); + }); + + it("ignores a non-boolean value rather than trusting it", () => { + window.__HF_CLI_CANARY_DECISIONS = { "on-everywhere": "false" } as unknown as Record< + string, + boolean + >; + // Falls through to local evaluation: on-everywhere is at 100%. + expect(resolveCanary("on-everywhere").enabled).toBe(true); + }); +}); diff --git a/packages/studio/src/telemetry/canary.ts b/packages/studio/src/telemetry/canary.ts index 943ed720f3..9572633e13 100644 --- a/packages/studio/src/telemetry/canary.ts +++ b/packages/studio/src/telemetry/canary.ts @@ -53,6 +53,34 @@ export function canaryParamName(name: string): string { declare global { interface Window { __HF_CLI_BUCKET_SEED?: string; + /** Resolved on/off per canary from the launching CLI. Authoritative. */ + __HF_CLI_CANARY_DECISIONS?: Record; + } +} + +/** + * The launching CLI's decision for this canary, if it published one. + * + * When present this WINS over everything Studio could work out locally — + * seed, URL override, registry percentage. Studio re-deriving cannot agree + * with the CLI in the cases that matter most: + * + * - telemetry off: the CLI resolves `telemetry_opt_out`, but Studio's + * opt-out is a separate localStorage flag it cannot see from here; + * - `HF_CANARY_*` override: env vars never cross into the browser; + * - no seed injected: Studio falls back to a different unit, i.e. a + * different bucket. + * + * In all three the CLI has already decided, and one render spanning both + * surfaces must not run half-enrolled. + */ +function cliDecision(name: string): boolean | undefined { + try { + if (typeof window === "undefined") return undefined; + const decision = window.__HF_CLI_CANARY_DECISIONS?.[name]; + return typeof decision === "boolean" ? decision : undefined; + } catch { + return undefined; } } @@ -142,6 +170,39 @@ export function __resetStudioCanaryCacheForTests(): void { decisions.clear(); } +/** The uncached decision. Split out so `resolveCanary` is purely the memo. */ +function decideStudioCanary(name: string): CanaryDecision { + const definition = findCanary(name); + if (!definition) return { enabled: false, reason: "out_of_cohort" }; + + // The launching CLI's decision, when there is one, is the whole answer: + // it already applied telemetry opt-out, HF_CANARY_* overrides and the + // percentage against the shared seed. Re-deriving here is what let the two + // surfaces disagree on the same render. + const fromCli = cliDecision(definition.name); + if (fromCli !== undefined) { + return { enabled: fromCli, reason: fromCli ? "forced_on" : "forced_off" }; + } + + const override = readOverride(definition.name); + // Standalone Studio. Opting out of telemetry opts you out of canaries — + // same rule as the CLI (see packages/cli/src/telemetry/canary.ts). A + // profile that sends nothing can't be compared against anyone, so + // enrolling it changes that user's code path for no signal. An explicit + // override still wins: a deliberate local choice, not silent enrolment. + if (override === undefined && isOptedOut()) { + return { enabled: false, reason: "telemetry_opt_out" }; + } + + return evaluateCanary({ + feature: definition.name, + unitId: resolveBucketUnit(), + percentage: definition.percentage, + override, + exclude: isAutomatedBrowser(), + }); +} + /** * Full decision including the reason. An unregistered name resolves to off * rather than throwing — a typo in a rollout control must never break the @@ -151,24 +212,7 @@ export function resolveCanary(name: string): CanaryDecision { const cached = decisions.get(name); if (cached) return cached; - const definition = findCanary(name); - const override = definition ? readOverride(definition.name) : undefined; - // Opting out of telemetry opts you out of canaries — same rule as the CLI - // (see packages/cli/src/telemetry/canary.ts). An install that sends nothing - // can't be compared against anyone, so enrolling it changes that user's - // code path for no signal. An explicit override still wins: that is a - // deliberate local choice, not silent enrolment. - const decision: CanaryDecision = !definition - ? { enabled: false, reason: "out_of_cohort" } - : override === undefined && isOptedOut() - ? { enabled: false, reason: "telemetry_opt_out" } - : evaluateCanary({ - feature: definition.name, - unitId: resolveBucketUnit(), - percentage: definition.percentage, - override, - exclude: isAutomatedBrowser(), - }); + const decision = decideStudioCanary(name); decisions.set(name, decision); return decision; From 31361b8e5b663c42869d723aea1fe4e42ce8329c Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 30 Jul 2026 16:38:11 -0700 Subject: [PATCH 16/20] fix(cli,core): close the remaining canary review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings from review, none behaviour-critical on their own but three of them quietly corrupt the data the rollout is judged by. Endpoint no longer serves bucketSeed (studioServer.ts). Studio gets its canary answers from the injected decisions map now, so nothing needed the seed over HTTP — and an unauthenticated local endpoint is a strictly worse place for it than a script scoped to Studio's own document. The endpoint itself predates this PR and still serves distinctId, so it also gains a Host guard: a remote page can rebind its hostname to 127.0.0.1 and read the response as same-origin, but the request still carries THAT hostname, which is what makes it refusable. predecessorFound no longer reports corruption as a fresh install. It returned null for both "file absent" and "file unreadable", so a partial disk write looked like a new machine — understating recoverable churn, the one thing the field measures. Now distinguishes absent from corrupt and emits install_state_file_corrupt alongside. A mangled markerAt no longer discards a salvageable bucketSeed. markerAt is only a timestamp and can be restamped; the seed cannot be recovered, and losing it silently re-rolls the install's cohort. The seed backfill no longer ignores its write result. An unwritable ~/.hyperframes meant a different seed every invocation with no diagnostic, and made the field's own "backfilled once" docstring false. Warns once per process with the underlying error. FNV-1a's ASCII constraint is now explicit rather than incidental. It hashes UTF-16 code units while reference FNV-1a is byte-oriented, so the two agree only on ASCII; the registry's kebab-case assertion is what makes non-ASCII unreachable, and both ends now say so. Not a live bug — names are kebab-case and units are UUIDs. de-parallel-router is pinned at 0%. The registry is data, so a ramp is a one-line edit with no review surface, and its own description says to ramp only alongside the circuit breaker. Tests: 8 new (corruption vs absence, seed salvage, backfill write failure, 17 host-guard cases, registry pin). One existing test asserted predecessorFound: false on corruption — that was the bug, updated with a note. Fault injection: restoring the old corrupt handling fails 4. Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/server/studioServer.ts | 22 ++- .../cli/src/server/telemetryIdentity.test.ts | 39 +++++- packages/cli/src/server/telemetryIdentity.ts | 26 +++- packages/cli/src/telemetry/client.ts | 4 + packages/cli/src/telemetry/config.test.ts | 81 ++++++++++- packages/cli/src/telemetry/config.ts | 126 ++++++++++++++---- packages/core/src/canary.test.ts | 24 +++- packages/core/src/canary.ts | 8 ++ 8 files changed, 294 insertions(+), 36 deletions(-) diff --git a/packages/cli/src/server/studioServer.ts b/packages/cli/src/server/studioServer.ts index 9bf7e97429..742bee1d28 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 { buildStudioHeadScripts, - resolveCliBucketSeed, + isLoopbackHost, resolveCliTelemetryDistinctId, } from "./telemetryIdentity.js"; import { emitStudioRenderComplete, emitStudioRenderError } from "./studioRenderTelemetry.js"; @@ -655,11 +655,23 @@ export function createStudioServer(options: StudioServerOptions): StudioServer { // clients that can't rely on the injected global. Returns the CLI's anonymous // distinct id (no PII) so the browser session can join the CLI's PostHog // person, or `{ distinctId: null }` when CLI telemetry is disabled. + // + // Deliberately does NOT serve `bucketSeed`. Studio gets its canary answers + // from the injected `window.__HF_CLI_CANARY_DECISIONS` (booleans, not the + // value cohorts derive from), so nothing needs the seed over HTTP — and an + // unauthenticated local endpoint is a strictly worse place for it than an + // inline script scoped to Studio's own document. + // + // Host-guarded against DNS rebinding: a remote page can point a hostname it + // controls at 127.0.0.1 and read this response as same-origin. Pinning the + // Host header to a loopback name means such a request (which carries the + // attacker's hostname) is refused. Same-origin Studio traffic always + // presents the bound loopback host. app.get("/api/telemetry-identity", (c) => { - return c.json({ - distinctId: resolveCliTelemetryDistinctId(), - bucketSeed: resolveCliBucketSeed(), - }); + if (!isLoopbackHost(c.req.header("host"))) { + return c.json({ error: "forbidden" }, 403); + } + return c.json({ distinctId: resolveCliTelemetryDistinctId() }); }); app.get("/api/events", (c) => { diff --git a/packages/cli/src/server/telemetryIdentity.test.ts b/packages/cli/src/server/telemetryIdentity.test.ts index 2791a31698..98728555bc 100644 --- a/packages/cli/src/server/telemetryIdentity.test.ts +++ b/packages/cli/src/server/telemetryIdentity.test.ts @@ -20,8 +20,12 @@ vi.mock("../telemetry/canary.js", () => ({ canaryDecisionsForStudio: () => canaryDecisions(), })); -const { resolveCliTelemetryDistinctId, buildCliIdentityScript, buildStudioHeadScripts } = - await import("./telemetryIdentity.js"); +const { + resolveCliTelemetryDistinctId, + buildCliIdentityScript, + buildStudioHeadScripts, + isLoopbackHost, +} = await import("./telemetryIdentity.js"); describe("resolveCliTelemetryDistinctId", () => { beforeEach(() => { @@ -167,3 +171,34 @@ describe("buildStudioHeadScripts", () => { expect(buildStudioHeadScripts(ENV_SCRIPT)).toBe(ENV_SCRIPT); }); }); + +describe("isLoopbackHost (DNS-rebinding guard on the identity endpoint)", () => { + it.each([ + "localhost", + "localhost:5173", + "127.0.0.1", + "127.0.0.1:5173", + "127.1.2.3", + "[::1]", + "[::1]:5173", + "LOCALHOST:5173", + ])("accepts the loopback host %s", (host) => { + expect(isLoopbackHost(host)).toBe(true); + }); + + it.each([ + undefined, + "", + // The rebinding case: an attacker hostname resolving to 127.0.0.1 still + // arrives with ITS name in Host, which is what makes this catchable. + "evil.example.com", + "evil.example.com:5173", + "127.0.0.1.evil.com", + "notlocalhost", + "localhost.evil.com", + "192.168.1.10", + "0.0.0.0", + ])("rejects %s", (host) => { + expect(isLoopbackHost(host)).toBe(false); + }); +}); diff --git a/packages/cli/src/server/telemetryIdentity.ts b/packages/cli/src/server/telemetryIdentity.ts index c9d843b1ed..707e259a19 100644 --- a/packages/cli/src/server/telemetryIdentity.ts +++ b/packages/cli/src/server/telemetryIdentity.ts @@ -43,7 +43,7 @@ export function resolveCliTelemetryDistinctId(): string | null { * and editor would split one user across cohorts. Same telemetry gate as the * distinct id: seeding is part of the identity stitch, not a separate channel. */ -export function resolveCliBucketSeed(): string | null { +function resolveCliBucketSeed(): string | null { try { if (!telemetryShouldTrack()) return null; const seed = readConfig().bucketSeed; @@ -53,6 +53,30 @@ export 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. + */ +export function isLoopbackHost(host: string | undefined): boolean { + if (!host) return false; + // Strip the port. IPv6 literals are bracketed (`[::1]:1234`), so take the + // bracketed part when present and only split on ":" otherwise. + const bracketed = /^\[([^\]]+)\]/.exec(host); + const hostname = (bracketed ? bracketed[1] : host.split(":")[0])?.toLowerCase() ?? ""; + if (hostname === "localhost" || hostname === "::1" || hostname === "0:0:0:0:0:0:0:1") return true; + // 127.0.0.0/8 — the whole loopback block, not just 127.0.0.1. + return /^127\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(hostname); +} + // JSON.stringify does not escape "<" or "/". Escaping both means no // "" (or " or open a new tag. (The values are diff --git a/packages/cli/src/telemetry/client.ts b/packages/cli/src/telemetry/client.ts index a1af000eae..1c9a577c5d 100644 --- a/packages/cli/src/telemetry/client.ts +++ b/packages/cli/src/telemetry/client.ts @@ -80,6 +80,10 @@ export function trackEvent( // we already knew. Absent (not false) when the config predates the // marker. Resolved after the shouldTrack guard. install_predecessor_found: readConfig().predecessorFound, + // Splits the `true` share above: a machine we knew but whose record we + // could not read. Without it a partial disk write is indistinguishable + // from a genuinely fresh install. Absent in the normal case. + install_state_file_corrupt: readConfig().stateFileCorrupt, // Canary assignments as `$feature/canary-` — PostHog's native flag // property shape, so breakdowns and experiment analysis work on a canary // with nothing configured server-side. On EVERY event, not just renders: diff --git a/packages/cli/src/telemetry/config.test.ts b/packages/cli/src/telemetry/config.test.ts index 2881f9778c..f2daadaddb 100644 --- a/packages/cli/src/telemetry/config.test.ts +++ b/packages/cli/src/telemetry/config.test.ts @@ -228,11 +228,15 @@ describe("install-state rollover (breaker survives a config re-mint)", () => { expect(stateFile()["markerAt"]).toBe(minted); }); - it("a corrupted state file reads as absent rather than breaking the mint", () => { + it("a corrupted state file never breaks the mint, and is not counted as fresh", () => { fsState.files.set(STATE_PATH, "{not valid json"); const config = readConfig(); - expect(config.predecessorFound).toBe(false); expect(config.anonymousId).toBeTruthy(); + // Previously asserted `false` here. That was the bug: a machine whose + // record we lost is not a new machine, and reporting it as one understated + // recoverable churn — the only thing predecessorFound measures. + expect(config.predecessorFound).toBe(true); + expect(config.stateFileCorrupt).toBe(true); // And the corrupt file was replaced with a valid marker by the mint's write. expect(stateFile()["markerAt"]).toEqual(expect.any(String)); }); @@ -388,3 +392,76 @@ describe("bucket-seed carryover (cohorts survive a config wipe)", () => { expect(readConfigFresh().bucketSeed).toBe(lineageSeed); }); }); + +describe("install-state corruption is distinguishable from absence", () => { + 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")); + }); + + it("reports predecessorFound for an unreadable state file, not a fresh install", () => { + fsState.files.set(STATE_PATH, "{not valid json"); + const config = readConfig(); + // The machine DID have an install; counting it as fresh understates + // recoverable churn, which is the only thing this field measures. + expect(config.predecessorFound).toBe(true); + expect(config.stateFileCorrupt).toBe(true); + }); + + it("leaves stateFileCorrupt absent on a genuinely fresh install", () => { + const config = readConfig(); + expect(config.predecessorFound).toBe(false); + expect(config.stateFileCorrupt).toBeUndefined(); + }); + + it("salvages a bucketSeed when only markerAt is mangled", () => { + // markerAt is regenerable; the seed is not — losing it re-rolls the cohort. + fsState.files.set(STATE_PATH, JSON.stringify({ markerAt: 12345, bucketSeed: "keep-me" })); + fsState.files.delete(CONFIG_PATH); + const config = readConfigFresh(); + expect(config.bucketSeed).toBe("keep-me"); + expect(config.predecessorFound).toBe(true); + }); + + it("treats a parseable file with nothing salvageable as corrupt", () => { + fsState.files.set(STATE_PATH, JSON.stringify({ unrelated: true })); + expect(readConfig().stateFileCorrupt).toBe(true); + }); +}); + +describe("seed backfill write failure is surfaced", () => { + let readConfig: typeof import("./config.js").readConfig; + let CONFIG_PATH: typeof import("./config.js").CONFIG_PATH; + + beforeEach(async () => { + fsState.files.clear(); + vi.resetModules(); + ({ readConfig, CONFIG_PATH } = await import("./config.js")); + }); + + it("warns once when the backfilled seed cannot be persisted", async () => { + // 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)); + const fs = await import("node:fs"); + vi.mocked(fs.writeFileSync).mockImplementation(() => { + throw new Error("EACCES: permission denied"); + }); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + readConfig(); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0]?.[0])).toContain("not be stable across runs"); + + 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 6482c563dc..690b6cd9ac 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -69,25 +69,87 @@ interface InstallState { bucketSeed?: string; } -/** Parse one state file; any parse/shape failure reads as absent. */ -function parseInstallState(file: string): InstallState | null { +/** + * Why there is no usable state: the file isn't there, or it is but couldn't be + * read/parsed. Distinguished because they mean opposite things for churn + * measurement — "absent" is a genuinely new machine, "corrupt" is a machine we + * already knew whose record we lost. Collapsing them into one `null` reported + * every corruption as a fresh install and biased `predecessorFound` low. + */ +type InstallStateMiss = "absent" | "corrupt"; + +/** + * Parse one state file. + * + * A malformed `markerAt` no longer discards the record. `markerAt` is + * regenerable — it is only a timestamp — while `bucketSeed` is not: losing it + * silently re-rolls the install's canary cohort. So a partial corruption that + * leaves a usable seed keeps the seed and restamps the marker. + */ +function salvageInstallState(parsed: Partial): InstallState | InstallStateMiss { + const markerAt = typeof parsed.markerAt === "string" ? parsed.markerAt : undefined; + const bucketSeed = parseNonEmptyString(parsed.bucketSeed); + // Nothing salvageable in the record at all — treat as corrupt rather than + // inventing a marker, so the miss is still counted as "we knew this machine" + // instead of masquerading as a fresh install. + if (markerAt === undefined && bucketSeed === undefined) return "corrupt"; + return { + markerAt: markerAt ?? new Date().toISOString(), + deParallelRouterTrialFired: parsed.deParallelRouterTrialFired === true ? true : undefined, + bucketSeed, + }; +} + +function parseInstallState(file: string): InstallState | InstallStateMiss { + if (!existsSync(file)) return "absent"; try { - if (!existsSync(file)) return null; - const parsed = JSON.parse(readFileSync(file, "utf-8")) as Partial; - if (typeof parsed.markerAt !== "string") return null; - return { - markerAt: parsed.markerAt, - deParallelRouterTrialFired: parsed.deParallelRouterTrialFired === true ? true : undefined, - bucketSeed: - typeof parsed.bucketSeed === "string" && parsed.bucketSeed.length > 0 - ? parsed.bucketSeed - : undefined, - }; + return salvageInstallState(JSON.parse(readFileSync(file, "utf-8")) as Partial); } catch { - return null; + return "corrupt"; } } +// Once per process: the backfill runs on every readConfig until it lands, and +// a read-only home directory would otherwise print on every single command. +let seedBackfillWarned = false; + +/** + * The seed backfill could not be persisted, so this install will mint a + * different seed next invocation and silently churn its canary cohort. Rare + * (unwritable ~/.hyperframes) but invisible without this, and it makes the + * "backfilled once" contract in the field's docstring false. + */ +function warnSeedBackfillFailed(error: string | undefined): void { + if (seedBackfillWarned) return; + seedBackfillWarned = true; + console.warn( + `[hyperframes] Could not persist telemetry config${error ? `: ${error}` : ""}. ` + + "Canary cohort assignment will not be stable across runs.", + ); +} + +/** + * One-time backfill for configs predating the bucket seed: prefer the seed a + * previous install recorded, else mint. Mutates `config` in place. + * + * Persisting is the whole point — an unpersisted seed re-rolls next process, + * silently churning this install's cohort on every invocation. `~/.hyperframes` + * being unwritable (root-owned after a sudo mishap, read-only mount, disk full) + * is exactly when that happens, so it says so once rather than failing + * invisibly. + */ +function backfillBucketSeed(config: HyperframesConfig): void { + const recorded = readInstallState(); + config.bucketSeed = (isInstallState(recorded) ? recorded.bucketSeed : undefined) ?? randomUUID(); + const write = writeConfigWithResult(config); + if (!write.ok) warnSeedBackfillFailed(write.error); +} + +/** Narrow the parse result to a usable record. */ +function isInstallState(value: InstallState | InstallStateMiss): value is InstallState { + return typeof value !== "string"; +} + /** * Read the install-state file, adopting the pre-move copy if this machine * still has one. @@ -97,14 +159,17 @@ function parseInstallState(file: string): InstallState | null { * cleared), and failing to delete the legacy copy is not an error — it is * re-read harmlessly next time. */ -function readInstallState(): InstallState | null { +function readInstallState(): InstallState | InstallStateMiss { const current = parseInstallState(STATE_FILE); - if (current !== null) { + if (isInstallState(current)) { removeLegacyStateFile(); return current; } const legacy = parseInstallState(LEGACY_STATE_FILE); - if (legacy === null) return null; + if (!isInstallState(legacy)) { + // Corruption at EITHER location still means this machine had an install. + return current === "corrupt" || legacy === "corrupt" ? "corrupt" : "absent"; + } try { writeInstallState(legacy); removeLegacyStateFile(); @@ -186,7 +251,8 @@ function syncInstallState(config: HyperframesConfig): void { const wantFired = config.deParallelRouterTrialFired === true; if (stateMarkerSynced && (stateFiredSynced || !wantFired)) return; try { - const state = readInstallState(); + const read = readInstallState(); + const state = isInstallState(read) ? read : null; const next = nextInstallState(state, config); if (next !== null) writeInstallState(next); stateMarkerSynced = true; @@ -202,15 +268,20 @@ function syncInstallState(config: HyperframesConfig): void { * machine left behind. */ function mintConfig(): HyperframesConfig { - const state = readInstallState(); + const read = readInstallState(); + const state = isInstallState(read) ? read : null; return { ...DEFAULT_CONFIG, anonymousId: randomUUID(), - predecessorFound: state !== null, + // A corrupt record still means this machine had an install — reporting it + // as `false` would count a partial disk write as a brand-new machine and + // understate recoverable churn, which is the one thing this field exists + // to measure. `undefined` on configs minted before the field existed. + predecessorFound: state !== null || read === "corrupt", + stateFileCorrupt: read === "corrupt" ? true : undefined, // The rollover itself: a breaker tripped by a previous install on this // machine stays tripped for the new one, and the canary bucketing seed is - // inherited so the machine keeps its cohorts — a wipe re-rolls the - // telemetry id, never the canary assignment. + // inherited so cohorts hold across a config.json re-mint. deParallelRouterTrialFired: state?.deParallelRouterTrialFired === true ? true : undefined, bucketSeed: state?.bucketSeed ?? randomUUID(), }; @@ -300,6 +371,13 @@ export interface HyperframesConfig { * existed — a different fact from `false` (minted fresh, no predecessor). */ predecessorFound?: boolean; + /** + * The install-state file existed but could not be read. Separates "we lost + * this machine's record" from "genuinely new machine" in the churn split — + * without it a partial disk write is indistinguishable from a fresh install. + * Absent (not false) in the normal case, so it costs nothing on the wire. + */ + stateFileCorrupt?: boolean; /** * The unit canary percentages bucket on — deliberately NOT the anonymousId. * A fresh random UUID, mirrored write-once into the install-state file and @@ -431,6 +509,7 @@ export function readConfig(): HyperframesConfig { : undefined, predecessorFound: typeof parsed.predecessorFound === "boolean" ? parsed.predecessorFound : undefined, + stateFileCorrupt: parsed.stateFileCorrupt === true ? true : undefined, bucketSeed: parseNonEmptyString(parsed.bucketSeed), recentRenders: parseRecentRenders(parsed.recentRenders), }; @@ -439,8 +518,7 @@ export function readConfig(): HyperframesConfig { // recorded seed if a previous install already wrote one, else mint. // Persisted immediately — an unpersisted seed would re-roll every process. if (config.bucketSeed === undefined) { - config.bucketSeed = readInstallState()?.bucketSeed ?? randomUUID(); - writeConfig(config); + backfillBucketSeed(config); // Cache even if the write failed, so the seed is at least stable for // the life of this process (a re-roll per readConfigFresh would flip // cohorts mid-session). diff --git a/packages/core/src/canary.test.ts b/packages/core/src/canary.test.ts index 78b038c325..f80cb2fa28 100644 --- a/packages/core/src/canary.test.ts +++ b/packages/core/src/canary.test.ts @@ -222,10 +222,30 @@ describe("parseCanaryOverride", () => { }); describe("registry", () => { - it("has unique, kebab-case names", () => { + // Also load-bearing for the hash, not just for tidiness: fnv1a32 walks + // charCodeAt, i.e. UTF-16 code units, while reference FNV-1a is byte + // oriented. The two agree only for ASCII. Names are hashed as + // `feature:unit`, so a non-ASCII name (an accented owner tag, an emoji, a + // full-width dash from autocorrect) would silently disagree with every + // other FNV-1a implementation — including any external tool that recomputes + // cohorts. This regex is what makes that unreachable; loosening it means + // fixing the hash first. + it("has unique, ASCII kebab-case names — the hash depends on this", () => { const names = CANARIES.map((c) => c.name); expect(new Set(names).size).toBe(names.length); - for (const n of names) expect(n).toMatch(/^[a-z0-9]+(-[a-z0-9]+)*$/); + for (const n of names) { + expect(n).toMatch(/^[a-z0-9]+(-[a-z0-9]+)*$/); + // eslint-disable-next-line no-control-regex -- explicit ASCII range check + expect(n).toMatch(/^[\x00-\x7F]*$/); + } + }); + + // The registry is data, so a ramp is a one-line edit with no code review + // surface. This canary's own description says "ramp only alongside the + // per-install circuit breaker" — without an assertion, bumping it to 5 + // before that wiring lands would go green. + it("keeps de-parallel-router at 0% until the circuit breaker is wired", () => { + expect(findCanary("de-parallel-router")?.percentage).toBe(0); }); it("has in-range percentages and a parseable sunset date", () => { diff --git a/packages/core/src/canary.ts b/packages/core/src/canary.ts index 10dd4fa123..cf8d40fb41 100644 --- a/packages/core/src/canary.ts +++ b/packages/core/src/canary.ts @@ -81,6 +81,14 @@ export interface CanaryInput { * run in the browser-side studio bundle and the embeddable player too, and a * six-line hash beats shipping a polyfill or maintaining two code paths. * Distribution is uniform enough for bucketing (pinned by a test). + * + * ASCII-only by contract. `charCodeAt` yields UTF-16 code units — two + * surrogate halves for an astral character — whereas reference FNV-1a is + * byte-oriented, so the two agree only on ASCII. Both inputs are constrained + * to satisfy that: canary names by the registry's kebab-case assertion in + * `canary.test.ts`, unit ids by being UUIDs. Widening either means switching + * to a UTF-8 encoding here first, which is not free in the browser bundle + * (`TextEncoder` is fine; `Buffer` is not). */ function fnv1a32(input: string): number { let hash = 0x811c9dc5; From 54534d53c257fda8fa0f14d4d91fd58724f6b6a6 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 30 Jul 2026 16:50:43 -0700 Subject: [PATCH 17/20] docs(canary): connect canary rollouts to the telemetry docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second half of a review comment I had only half-addressed: the opt-out behaviour shipped in 4f464dc4, but "should we add this into/connected to the existing telemetry doc?" did not. There is no standalone telemetry page — the canonical disclosure is the `telemetry` command section in packages/cli. Links now run both ways so neither page describes canaries as a separate system: - packages/cli #telemetry: telemetry state also controls canary enrolment, with every opt-out route named. - contributing/canary-rollouts: a Note at the top, before any of the how-to, saying canaries sit behind the telemetry switch and why (a canary is a measured rollout; an install that reports nothing cannot be compared, so enrolling it buys no signal). - guides/feedback "Opting Out": disabling telemetry also ends canary enrolment, alongside the feedback prompt and usage tracking. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 12 ++++++++++++ docs/guides/feedback.mdx | 2 +- docs/packages/cli.mdx | 7 +++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index 58981a4dbf..5d11c0c6ef 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -8,6 +8,18 @@ exercised on real traffic) or on for everyone (and therefore a fleet-wide bet). A canary is the rung in between — the same change, enabled for a stable slice of installs, ramped as the signal holds. + + **Canaries are part of telemetry, not a separate system.** A canary is a + *measured* rollout: a slice is enrolled specifically so it can be compared + against everyone else, and the comparison is what makes ramping safe. An + install that reports nothing cannot be compared, so **opting out of telemetry + opts you out of canaries** — via `hyperframes telemetry disable`, + `HYPERFRAMES_NO_TELEMETRY=1`, `DO_NOT_TRACK=1`, a dev build, or Studio's + `hyperframes-studio:telemetryDisabled`. See the + [`telemetry` command](/packages/cli#telemetry) for what is collected and how + to turn it off; everything on this page sits behind that switch. + + ## Add one **1. Register it at 0%.** In `packages/core/src/canaryRegistry.ts`: diff --git a/docs/guides/feedback.mdx b/docs/guides/feedback.mdx index f7f70231ef..5d5a397119 100644 --- a/docs/guides/feedback.mdx +++ b/docs/guides/feedback.mdx @@ -199,7 +199,7 @@ Studio stores the equivalent state in `localStorage` under the `hyperframes-stud ### CLI — disable telemetry entirely -Disabling telemetry suppresses the CLI feedback prompt and all other CLI usage tracking: +Disabling telemetry suppresses the CLI feedback prompt, all other CLI usage tracking, and enrolment in any [canary rollout](/contributing/canary-rollouts) — an install that reports nothing is never picked for a staged rollout: ```bash # Via CLI command (persisted to ~/.hyperframes/config.json) diff --git a/docs/packages/cli.mdx b/docs/packages/cli.mdx index efd4b331a2..ab704e8b40 100644 --- a/docs/packages/cli.mdx +++ b/docs/packages/cli.mdx @@ -992,6 +992,13 @@ Word-level transcripts (whisper output) are grouped into readable caption cues o Telemetry collects command names, render performance, render checkpoint/error names, aggregate browser diagnostic counts, browser initialization duration and tween count, aggregate video extraction workload counts (such as extracted frame count and VFR preflight count), example choices, and system info — including a coarse environment fingerprint (OS, kernel string, CPU/memory shape, sandbox runtime such as gVisor or Docker, and the *name* of a coding agent driving the CLI when one is detected, e.g. `claude_code` / `codex` / `cursor`). The agent name is derived from the existence of well-known environment variables; their values are never read. Telemetry redacts local paths and URL query strings from render error/checkpoint messages and does **not** collect project names, video content, or environment variable values. It collects no personally identifiable information until you sign in: when you authenticate with `hyperframes auth login`, your HeyGen account email (or your username, if your account has no email) is linked to your usage so CLI activity can be associated with your account (and your prior anonymous usage is stitched to it). Nothing else personal is collected, and this only happens after you choose to sign in. Disable all telemetry with `HYPERFRAMES_NO_TELEMETRY=1` or the command above. + Telemetry state also controls **canary enrolment**: staged rollouts pick a + stable slice of installs to enable a change for, and an install that reports + nothing cannot be compared against anyone, so opting out of telemetry opts you + out of canaries too. Every route counts — `hyperframes telemetry disable`, + `HYPERFRAMES_NO_TELEMETRY=1`, `DO_NOT_TRACK=1`, and dev builds. See + [Canary rollouts](/contributing/canary-rollouts). + See [Feedback Collection](/guides/feedback) for how the periodic post-render prompt and Studio feedback bar work, what data they collect, and how to opt out. ### `skills` From f81ab0162e2add6c278518de8dc237450b5f8d93 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 30 Jul 2026 18:35:22 -0700 Subject: [PATCH 18/20] fix(cli,studio): close the four R3 blocking gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 — SPA route bypassed the DNS-rebinding guard. Guarding only /api/telemetry-identity left the catch-all as an open side door: a rebound origin could fetch `/` and read __HF_CLI_DISTINCT_ID and __HF_CLI_BUCKET_SEED straight out of the returned HTML. The SPA response now applies the same isLoopbackHost() check; an untrusted Host still gets a working Studio, just with no identity, seed or decisions injected. Route-level regression added. P1 — a CLI cohort roll could override Studio's own opt-out. decideStudioCanary() adopted the injected decision before checking isOptedOut(), so CLI-telemetry-on plus Studio-opted-out still enrolled Studio. A bare boolean could not express the difference between a deliberate override and an ordinary cohort roll, so the injected map now carries provenance ({ enabled, forced }). Forced wins outright — it is the documented escalation channel and must behave the same on both surfaces — while a percentage roll now loses to this profile's opt-out. Full interaction matrix tested. P1 — the legacy studio:* path sat outside both contracts. utils/studioTelemetry.ts shipped its own opt-out key and its own send loop, so the documented hyperframes-studio:telemetryDisabled did not silence it and its events carried no cohort assignment. It now honours both keys (the legacy one stays, so nobody already opted out is quietly re-enabled) and mixes in canaryEventProperties(), making "every telemetry event carries the assignment" actually true. P2 — partial salvage could drop a tripped breaker. salvageInstallState() discarded the whole record when markerAt and bucketSeed were both unusable, taking deParallelRouterTrialFired with it and re-enrolling a machine whose router already failed. All three fields are now independently salvageable. Docs: canary-rollouts.mdx said "disabling telemetry disables the reporting, not the enrolment" — exactly backwards since the opt-out gate landed. Corrected; checked for other copies, none. Tests: 13 new (4 opt-out precedence, 4 legacy-path opt-out and canary props, 3 route-level host guard, 2 breaker salvage). Fault injection: each of the four fixes reverted independently fails its own tests (2 CLI + 1 Studio + 2 Studio). Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 4 +- packages/cli/src/server/studioServer.test.ts | 54 ++++++++++++++ packages/cli/src/server/studioServer.ts | 12 ++- .../cli/src/server/telemetryIdentity.test.ts | 16 ++-- packages/cli/src/server/telemetryIdentity.ts | 4 +- packages/cli/src/telemetry/canary.ts | 26 ++++++- packages/cli/src/telemetry/config.test.ts | 29 ++++++++ packages/cli/src/telemetry/config.ts | 12 +-- packages/studio/src/telemetry/canary.test.ts | 54 +++++++++++--- packages/studio/src/telemetry/canary.ts | 58 ++++++++++----- .../studio/src/utils/studioTelemetry.test.ts | 74 +++++++++++++++++++ packages/studio/src/utils/studioTelemetry.ts | 17 ++++- 12 files changed, 313 insertions(+), 47 deletions(-) create mode 100644 packages/studio/src/utils/studioTelemetry.test.ts diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index 5d11c0c6ef..cda386b609 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -80,7 +80,9 @@ tooling, so whoever operates the telemetry backend can split any metric by cohort with **nothing configured server-side** — while the decision itself still happens locally and offline, which the render path requires. (Assignments ride the same anonymous, opt-out telemetry pipeline as every -other event; disabling telemetry disables the reporting, not the enrolment.) +other event — and disabling telemetry disables the **enrolment**, not just the +reporting: an opted-out install is never bucketed at all. See the note at the +top of this page.) Two details worth knowing: diff --git a/packages/cli/src/server/studioServer.test.ts b/packages/cli/src/server/studioServer.test.ts index 466317aff3..cb5ce853c0 100644 --- a/packages/cli/src/server/studioServer.test.ts +++ b/packages/cli/src/server/studioServer.test.ts @@ -57,3 +57,57 @@ describe("createStudioServer autoProxy plumbing", () => { expect(server.adapter.autoProxy).toBe(true); }); }); + +describe("host guarding on identity-bearing responses", () => { + const dirs: string[] = []; + let server: StudioServer | undefined; + + function tmpProject(): string { + const dir = mkdtempSync(join(tmpdir(), "hf-studio-host-test-")); + dirs.push(dir); + return dir; + } + + afterEach(() => { + server?.watcher.close(); + server = undefined; + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + // A rebound origin can point its own hostname at 127.0.0.1 and read + // responses as same-origin. Guarding only /api/telemetry-identity left the + // SPA route as an open side door: fetching `/` returned the same distinct + // id and bucket seed inline in the HTML. + it("omits identity injection from the SPA response for a hostile Host", async () => { + server = createStudioServer({ projectDir: tmpProject() }); + const res = await server.app.request("/", { headers: { host: "evil.example.com" } }); + const html = await res.text(); + expect(html).not.toContain("__HF_CLI_DISTINCT_ID"); + expect(html).not.toContain("__HF_CLI_BUCKET_SEED"); + expect(html).not.toContain("__HF_CLI_CANARY_DECISIONS"); + // Studio still loads — only the identity block is withheld. (The env + // script is empty here: it only emits with VITE_STUDIO_* vars set.) + expect(res.status).toBe(200); + expect(html).toContain(""); + }); + + it("refuses the identity endpoint for a hostile Host", async () => { + server = createStudioServer({ projectDir: tmpProject() }); + const res = await server.app.request("/api/telemetry-identity", { + headers: { host: "evil.example.com" }, + }); + expect(res.status).toBe(403); + expect(await res.text()).not.toContain('distinctId":"'); + }); + + it("serves the identity endpoint on a loopback Host", async () => { + server = createStudioServer({ projectDir: tmpProject() }); + const res = await server.app.request("/api/telemetry-identity", { + headers: { host: "127.0.0.1:5173" }, + }); + expect(res.status).toBe(200); + // The seed is no longer served here at all — Studio gets decisions + // injected instead, so nothing needs it over HTTP. + expect(Object.keys((await res.json()) as object)).toEqual(["distinctId"]); + }); +}); diff --git a/packages/cli/src/server/studioServer.ts b/packages/cli/src/server/studioServer.ts index 742bee1d28..210ce82f7f 100644 --- a/packages/cli/src/server/studioServer.ts +++ b/packages/cli/src/server/studioServer.ts @@ -814,7 +814,17 @@ export function createStudioServer(options: StudioServerOptions): StudioServer { // Inject before the studio bundle runs. Identity script first (see // buildStudioHeadScripts) so the CLI distinct id is on `window` by the time // telemetry init reads it. - const headScript = buildStudioHeadScripts(buildRuntimeEnvScript()); + // + // Host-guarded for the same reason /api/telemetry-identity is, and it has + // to be checked HERE too: guarding only the endpoint leaves this route as + // an open side door, since a rebound origin can simply fetch `/` and read + // the same distinct id and seed out of the returned HTML. Untrusted Host + // still gets a working Studio — it just gets the env script alone, with no + // identity, no seed, and no canary decisions. + const trustedHost = isLoopbackHost(c.req.header("host")); + const headScript = trustedHost + ? buildStudioHeadScripts(buildRuntimeEnvScript()) + : buildRuntimeEnvScript(); if (headScript) { html = html.replace("", `${headScript}`); } diff --git a/packages/cli/src/server/telemetryIdentity.test.ts b/packages/cli/src/server/telemetryIdentity.test.ts index 98728555bc..4599fa3b3b 100644 --- a/packages/cli/src/server/telemetryIdentity.test.ts +++ b/packages/cli/src/server/telemetryIdentity.test.ts @@ -8,7 +8,7 @@ const shouldTrack = vi.fn(); const readConfig = vi.fn(); // Pinned rather than using the real registry, so these string assertions // don't move every time a canary is added, ramped, or retired. -const canaryDecisions = vi.fn<() => Record>(); +const canaryDecisions = vi.fn<() => Record>(); vi.mock("../telemetry/client.js", () => ({ shouldTrack: (...args: unknown[]) => shouldTrack(...args), @@ -99,10 +99,11 @@ describe("buildCliIdentityScript", () => { // stops Studio evaluating independently and enrolling anyway. it("still publishes canary decisions when telemetry is off, but no identity", () => { shouldTrack.mockReturnValue(false); - canaryDecisions.mockReturnValue({ "de-parallel-router": false }); + canaryDecisions.mockReturnValue({ "de-parallel-router": { enabled: false, forced: false } }); const script = buildCliIdentityScript(); expect(script).toBe( - '', + "', ); expect(script).not.toContain("__HF_CLI_DISTINCT_ID"); expect(script).not.toContain("__HF_CLI_BUCKET_SEED"); @@ -111,17 +112,20 @@ describe("buildCliIdentityScript", () => { it("publishes decisions alongside the identity when telemetry is on", () => { shouldTrack.mockReturnValue(true); readConfig.mockReturnValue({ anonymousId: "machine-uuid", bucketSeed: "seed-uuid" }); - canaryDecisions.mockReturnValue({ "de-parallel-router": true }); + canaryDecisions.mockReturnValue({ "de-parallel-router": { enabled: true, forced: true } }); expect(buildCliIdentityScript()).toBe( '', + "window.__HF_CLI_CANARY_DECISIONS=" + + '{"de-parallel-router":{"enabled":true,"forced":true}};', ); }); it("escapes a canary name that tries to close the script tag", () => { shouldTrack.mockReturnValue(false); - canaryDecisions.mockReturnValue({ ""; + + beforeEach(() => { + shouldTrack.mockReset(); + readConfig.mockReset(); + canaryDecisions.mockReset(); + shouldTrack.mockReturnValue(true); + readConfig.mockReturnValue({ anonymousId: "machine-uuid", bucketSeed: "seed-uuid" }); + canaryDecisions.mockReturnValue({ "de-parallel-router": { enabled: true, forced: false } }); + }); + + it("publishes identity and decisions on a loopback Host", () => { + const head = buildStudioHeadScriptsForHost(ENV, "127.0.0.1:5173"); + expect(head).toContain("__HF_CLI_DISTINCT_ID"); + expect(head).toContain("__HF_CLI_BUCKET_SEED"); + expect(head).toContain("__HF_CLI_CANARY_DECISIONS"); + }); + + // DNS rebinding: identity must not be readable from a hostile origin. + it("withholds identity and seed from a hostile Host", () => { + const head = buildStudioHeadScriptsForHost(ENV, "evil.example.com"); + expect(head).not.toContain("__HF_CLI_DISTINCT_ID"); + expect(head).not.toContain("__HF_CLI_BUCKET_SEED"); + }); + + // ...but the decisions map is NOT identifying, and withholding it would send + // a supported LAN preview (HYPERFRAMES_PREVIEW_HOST=0.0.0.0) back to + // re-deriving locally and disagreeing with the CLI. + it.each(["evil.example.com", "192.168.1.10:5173", "my-dev-box.local:5173", undefined])( + "still publishes canary decisions for non-loopback Host %s", + (host) => { + const head = buildStudioHeadScriptsForHost(ENV, host); + expect(head).toContain("__HF_CLI_CANARY_DECISIONS"); + expect(head).not.toContain("__HF_CLI_DISTINCT_ID"); + }, + ); + + it("always keeps the env script, whatever the Host", () => { + expect(buildStudioHeadScriptsForHost(ENV, "evil.example.com")).toContain("__HF_STUDIO_ENV__"); + expect(buildStudioHeadScriptsForHost(ENV, "localhost")).toContain("__HF_STUDIO_ENV__"); + }); +}); diff --git a/packages/cli/src/server/telemetryIdentity.ts b/packages/cli/src/server/telemetryIdentity.ts index ad53b54a75..f8d027274c 100644 --- a/packages/cli/src/server/telemetryIdentity.ts +++ b/packages/cli/src/server/telemetryIdentity.ts @@ -119,10 +119,16 @@ function resolveCliCanaryDecisions(): Record | null { * or browser history. Empty string only when there is nothing at all to * publish. */ -export function buildCliIdentityScript(): string { +export function buildCliIdentityScript(options: { includeIdentity?: boolean } = {}): string { + const { includeIdentity = true } = options; const parts: string[] = []; - const cliId = resolveCliTelemetryDistinctId(); + // Identity is the only part gated on a trusted Host. The decisions map below + // is not identifying, and withholding it would push a LAN/remote Studio + // (`HYPERFRAMES_PREVIEW_HOST=0.0.0.0`, an explicitly supported mode) back to + // re-deriving locally — reopening exactly the CLI/Studio disagreement this + // whole mechanism exists to close. + const cliId = includeIdentity ? resolveCliTelemetryDistinctId() : null; if (cliId) { parts.push(`window.__HF_CLI_DISTINCT_ID=${encodeInlineScriptValue(cliId)};`); const seed = resolveCliBucketSeed(); @@ -154,6 +160,21 @@ export function buildCliIdentityScript(): string { * ordering in one pure, tested function guards against a future `` inject * silently landing ahead of the identity script and reintroducing a boot race. */ -export function buildStudioHeadScripts(envScript: string): string { - return `${buildCliIdentityScript()}${envScript}`; +export function buildStudioHeadScripts( + envScript: string, + options: { includeIdentity?: boolean } = {}, +): string { + return `${buildCliIdentityScript(options)}${envScript}`; +} + +/** + * The `` scripts for a request, given its `Host`. + * + * The Host split lives here rather than in the route so it is testable + * without a Studio bundle on disk — the route's own test can only reach the + * injection branch when `packages/studio/dist` happens to be built, which is + * true locally and false in the CI test lane. + */ +export function buildStudioHeadScriptsForHost(envScript: string, host: string | undefined): string { + return buildStudioHeadScripts(envScript, { includeIdentity: isLoopbackHost(host) }); } diff --git a/packages/cli/src/telemetry/config.test.ts b/packages/cli/src/telemetry/config.test.ts index 9f0dd1c295..09b42ff46a 100644 --- a/packages/cli/src/telemetry/config.test.ts +++ b/packages/cli/src/telemetry/config.test.ts @@ -494,3 +494,62 @@ describe("a tripped breaker survives even total marker+seed corruption", () => { expect(readConfig().stateFileCorrupt).toBe(true); }); }); + +describe("the breaker latch is authoritative from install-state", () => { + let readConfig: typeof import("./config.js").readConfig; + let writeConfigWithResult: typeof import("./config.js").writeConfigWithResult; + 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, writeConfigWithResult, CONFIG_PATH, STATE_PATH } = await import("./config.js")); + }); + + // The stale-writer race: a concurrent process rewrites config.json from a + // snapshot taken before the breaker fired, clearing the flag there. Without + // merging the latch on read, the next process re-enrols a machine whose + // router already failed. + it("rehydrates a latched breaker when config.json says otherwise", () => { + fsState.files.set( + STATE_PATH, + JSON.stringify({ markerAt: "2026-07-28T00:00:00.000Z", deParallelRouterTrialFired: true }), + ); + fsState.files.set( + CONFIG_PATH, + JSON.stringify({ telemetryEnabled: true, anonymousId: "id", bucketSeed: "seed" }), + ); + expect(readConfig().deParallelRouterTrialFired).toBe(true); + }); + + it("does not invent a latch when neither store has one", () => { + 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(); + }); + + it("reports mirrored:false when the state write fails but config.json lands", async () => { + const config = readConfig(); + const fs = await import("node:fs"); + const { __resetInstallStateSyncForTests } = await import("./config.js"); + __resetInstallStateSyncForTests(); + fsState.files.delete(STATE_PATH); + vi.mocked(fs.writeFileSync).mockImplementation((path, content) => { + if (String(path).startsWith(STATE_PATH)) throw new Error("EACCES"); + fsState.files.set(String(path), String(content)); + }); + + config.deParallelRouterTrialFired = true; + // The config write still succeeds — a failed mirror must never break it — + // but the caller can now see the safety fact reached only one store. + expect(writeConfigWithResult(config)).toEqual({ ok: true, mirrored: false }); + + 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 ffbbb365f9..30eb92451c 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -147,6 +147,29 @@ 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; + +/** + * Is the breaker latched according to install-state? + * + * Merged into EVERY effective read, which is what makes install-state + * authoritative for the latch rather than merely a mirror of config.json. + * Without this a failed mirror, or a stale concurrent writer that rewrites + * config.json from a pre-trip snapshot, leaves state latched and config + * false — and the next process happily re-enrols a machine whose router + * already failed. Verifying the write is not enough on its own; the read has + * to prefer the safety fact. + */ +function installStateLatchedFired(): boolean { + if (latchFromStateFile === undefined) { + const state = readInstallState(); + latchFromStateFile = isInstallState(state) && state.deParallelRouterTrialFired === true; + } + return latchFromStateFile; +} + /** Narrow the parse result to a usable record. */ function isInstallState(value: InstallState | InstallStateMiss): value is InstallState { return typeof value !== "string"; @@ -249,18 +272,27 @@ function nextInstallState( return next; } -function syncInstallState(config: HyperframesConfig): void { +/** @returns false if the mirror could not be written this call. */ +/** The write half, split out to keep the memo bookkeeping legible. */ +function applyInstallState(config: HyperframesConfig, wantFired: boolean): void { + const read = readInstallState(); + const state = isInstallState(read) ? read : null; + const next = nextInstallState(state, config); + if (next !== null) writeInstallState(next); + stateMarkerSynced = true; + stateFiredSynced = wantFired || state?.deParallelRouterTrialFired === true; + if (wantFired) latchFromStateFile = true; +} + +function syncInstallState(config: HyperframesConfig): boolean { const wantFired = config.deParallelRouterTrialFired === true; - if (stateMarkerSynced && (stateFiredSynced || !wantFired)) return; + if (stateMarkerSynced && (stateFiredSynced || !wantFired)) return true; try { - const read = readInstallState(); - const state = isInstallState(read) ? read : null; - const next = nextInstallState(state, config); - if (next !== null) writeInstallState(next); - stateMarkerSynced = true; - stateFiredSynced = wantFired || state?.deParallelRouterTrialFired === true; + applyInstallState(config, wantFired); + return true; } catch { // Leave the memo unset so a later write retries. + return false; } } @@ -504,7 +536,10 @@ export function readConfig(): HyperframesConfig { // 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). - deParallelRouterTrialFired: parsed.deParallelRouterTrialFired === true ? true : undefined, + // `|| 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 @@ -575,7 +610,11 @@ export function writeConfig(config: HyperframesConfig): boolean { return writeConfigWithResult(config).ok; } -export type ConfigWriteResult = { ok: true } | { ok: false; error: string }; +export type ConfigWriteResult = + // `mirrored: false` means config.json landed but the install-state mirror + // did not. Not a write failure — the latch is merged back in on read — but + // callers persisting a tripped breaker may want to retry or warn. + { ok: true; mirrored?: false } | { ok: false; error: string }; /** * Persist config and retain the failure reason for user-facing commands that @@ -589,9 +628,13 @@ export function writeConfigWithResult(config: HyperframesConfig): ConfigWriteRes renameSync(tmpFile, CONFIG_FILE); cachedConfig = { ...config }; // Mirror into the install-state file (marker + tripped breaker) so no - // breaker write site has to remember to do it. - syncInstallState(config); - return { ok: true }; + // breaker write site has to remember to do it. A failed mirror does NOT + // fail the config write — config.json landed, and the latch is merged + // back in on read — but it is reported rather than swallowed so a caller + // persisting a tripped breaker can tell the safety fact only reached one + // of the two stores. + const mirrored = syncInstallState(config); + return mirrored ? { ok: true } : { ok: true, mirrored: false }; } catch (error) { // Non-fatal — telemetry should never break the CLI return { ok: false, error: normalizeErrorMessage(error) }; diff --git a/packages/studio/src/telemetry/canary.test.ts b/packages/studio/src/telemetry/canary.test.ts index c6bc0c5d04..802a4aaf2b 100644 --- a/packages/studio/src/telemetry/canary.test.ts +++ b/packages/studio/src/telemetry/canary.test.ts @@ -6,6 +6,14 @@ import { evaluateCanary } from "@hyperframes/core/canary"; // Pin the registry: real entries move as rollouts ramp, and these tests are // about the BINDING (does the browser supply the right three inputs?), not // about whichever canaries happen to be live today. +// The policy reads import.meta.env.DEV, which vitest sets true — without +// this every case would resolve to telemetry_opt_out. Controlled explicitly +// so each test states the privacy posture it is exercising. +const policyState = { allowed: true }; +vi.mock("./policy", () => ({ + browserTelemetryAllowed: () => policyState.allowed, +})); + vi.mock("@hyperframes/core/canary-registry", async () => { const actual = await vi.importActual( "@hyperframes/core/canary-registry", @@ -43,6 +51,7 @@ function setSearch(search: string): void { } beforeEach(() => { + policyState.allowed = true; localStorage.clear(); sessionStorage.clear(); setSearch(""); @@ -198,6 +207,7 @@ describe("telemetry opt-out is canary opt-out", () => { const OPT_OUT_KEY = "hyperframes-studio:telemetryDisabled"; it("does not enrol an opted-out browser profile", () => { + policyState.allowed = false; localStorage.setItem(OPT_OUT_KEY, "1"); // on-everywhere is at 100% — it would be on for everyone otherwise. expect(resolveCanary("on-everywhere")).toEqual({ @@ -207,17 +217,20 @@ describe("telemetry opt-out is canary opt-out", () => { }); it("never buckets an opted-out profile — no cohort is assigned at all", () => { + policyState.allowed = false; localStorage.setItem(OPT_OUT_KEY, "1"); expect(resolveCanary("on-everywhere").bucket).toBeUndefined(); }); it("still honours an explicit URL override", () => { + policyState.allowed = false; localStorage.setItem(OPT_OUT_KEY, "1"); setSearch("?hf_canary_off_everywhere=on"); expect(resolveCanary("off-everywhere")).toEqual({ enabled: true, reason: "forced_on" }); }); it("reports every canary as false when opted out", () => { + policyState.allowed = false; localStorage.setItem(OPT_OUT_KEY, "1"); expect(canaryEventProperties()).toEqual({ "$feature/canary-on-everywhere": "false", @@ -281,6 +294,7 @@ describe("CLI-launched Studio adopts the CLI's decisions", () => { // opt-outs, and CLI telemetry being on says nothing about this profile. describe("precedence against Studio's own opt-out", () => { beforeEach(() => { + policyState.allowed = false; localStorage.setItem(OPT_OUT_KEY, "1"); }); diff --git a/packages/studio/src/telemetry/canary.ts b/packages/studio/src/telemetry/canary.ts index a8ef6a644d..8c90f8088c 100644 --- a/packages/studio/src/telemetry/canary.ts +++ b/packages/studio/src/telemetry/canary.ts @@ -41,7 +41,7 @@ import { } from "@hyperframes/core/canary"; import { CANARIES, findCanary } from "@hyperframes/core/canary-registry"; import { resolveStudioDistinctId } from "./distinctId"; -import { isOptedOut } from "./config"; +import { browserTelemetryAllowed } from "./policy"; import { safeSessionStorage } from "../utils/safeStorage"; /** `my-feature` → `hf_canary_my_feature`, the query param and storage key. */ @@ -212,7 +212,7 @@ function decideStudioCanary(name: string): CanaryDecision { const override = readOverride(definition.name); if (override === undefined) { - if (isOptedOut()) return { enabled: false, reason: "telemetry_opt_out" }; + if (!browserTelemetryAllowed()) return { enabled: false, reason: "telemetry_opt_out" }; if (fromCli !== undefined) return cohortOutcome(fromCli.enabled); } diff --git a/packages/studio/src/telemetry/client.ts b/packages/studio/src/telemetry/client.ts index f43aa3486a..511571bb47 100644 --- a/packages/studio/src/telemetry/client.ts +++ b/packages/studio/src/telemetry/client.ts @@ -4,7 +4,8 @@ // All calls are fire-and-forget; telemetry must never break the studio UI. // --------------------------------------------------------------------------- -import { getAnonymousId, hasShownNotice, isOptedOut, markNoticeShown } from "./config"; +import { getAnonymousId, hasShownNotice, markNoticeShown } from "./config"; +import { browserTelemetryAllowed } from "./policy"; import { getBrowserSystemMeta } from "./system"; import { canaryEventProperties } from "./canary"; @@ -25,46 +26,11 @@ let eventQueue: QueuedEvent[] = []; let flushTimer: ReturnType | null = null; let telemetryEnabled: boolean | null = null; -function isDoNotTrackOn(): boolean { - return typeof navigator !== "undefined" && navigator.doNotTrack === "1"; -} - -function isApiKeyConfigured(): boolean { - return POSTHOG_API_KEY.startsWith("phc_"); -} - -// VITE_HYPERFRAMES_NO_TELEMETRY mirrors the CLI's HYPERFRAMES_NO_TELEMETRY=1 -// opt-out so HeyGen's own dev/CI builds can suppress telemetry from the studio -// bundle the same way. Vite injects it at build time. Match the CLI's -// affirmative privacy-control spellings. -// `import.meta.env` may be undefined in non-Vite bundlers (Next.js Turbopack). -function isBuildTimeOptOut(): boolean { - try { - const v = import.meta.env.VITE_HYPERFRAMES_NO_TELEMETRY as string | undefined; - return v !== undefined && ["1", "true", "yes", "on"].includes(v.trim().toLowerCase()); - } catch { - return false; - } -} - -// `import.meta.env.DEV` is true under `vite dev` / `vite preview`. Auto-suppress -// so developers running `hyperframes preview` don't pollute production telemetry. -function isViteDevMode(): boolean { - try { - return import.meta.env.DEV === true; - } catch { - return false; - } -} - export function shouldTrack(): boolean { if (telemetryEnabled !== null) return telemetryEnabled; - telemetryEnabled = - isApiKeyConfigured() && - !isBuildTimeOptOut() && - !isViteDevMode() && - !isOptedOut() && - !isDoNotTrackOn(); + // Delegated to telemetry/policy.ts so this transport, the older `studio:*` + // transport, and canary enrolment cannot drift apart again. + telemetryEnabled = browserTelemetryAllowed(); return telemetryEnabled; } diff --git a/packages/studio/src/telemetry/policy.test.ts b/packages/studio/src/telemetry/policy.test.ts new file mode 100644 index 0000000000..e11bbbfaec --- /dev/null +++ b/packages/studio/src/telemetry/policy.test.ts @@ -0,0 +1,81 @@ +// @vitest-environment happy-dom +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +// Each control the browser telemetry policy enforces, asserted individually. +// This is the SSOT that `telemetry/client.ts`, `utils/studioTelemetry.ts` and +// canary enrolment all consult, so a gap here is a gap in all three — which is +// how `navigator.doNotTrack` and Vite dev mode came to suppress one transport +// but not the other, nor enrolment. + +const DOCUMENTED_OPT_OUT = "hyperframes-studio:telemetryDisabled"; +const LEGACY_OPT_OUT = "hf-studio-telemetry-opt-out"; + +describe("browserTelemetryAllowed", () => { + let browserTelemetryAllowed: typeof import("./policy").browserTelemetryAllowed; + + beforeEach(async () => { + localStorage.clear(); + vi.resetModules(); + // vitest sets import.meta.env.DEV; the policy suppresses under it, so the + // baseline has to be an explicitly production-like env. + vi.stubEnv("DEV", false); + vi.stubEnv("VITE_HYPERFRAMES_NO_TELEMETRY", ""); + Object.defineProperty(navigator, "doNotTrack", { value: null, configurable: true }); + ({ browserTelemetryAllowed } = await import("./policy")); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("allows telemetry with no control set", () => { + expect(browserTelemetryAllowed()).toBe(true); + }); + + it("refuses when the documented localStorage key is set", () => { + localStorage.setItem(DOCUMENTED_OPT_OUT, "1"); + expect(browserTelemetryAllowed()).toBe(false); + }); + + // Anyone already opted out this way must never be quietly re-enabled by the + // move to the documented key. + it("refuses when the legacy localStorage key is set", () => { + localStorage.setItem(LEGACY_OPT_OUT, "1"); + expect(browserTelemetryAllowed()).toBe(false); + }); + + it("refuses when navigator.doNotTrack is on", () => { + Object.defineProperty(navigator, "doNotTrack", { value: "1", configurable: true }); + expect(browserTelemetryAllowed()).toBe(false); + }); + + it("refuses under Vite dev mode", async () => { + vi.stubEnv("DEV", true); + vi.resetModules(); + ({ browserTelemetryAllowed } = await import("./policy")); + expect(browserTelemetryAllowed()).toBe(false); + }); + + it.each(["1", "true", "yes", "on", " ON "])( + "refuses when VITE_HYPERFRAMES_NO_TELEMETRY=%s", + async (value) => { + vi.stubEnv("VITE_HYPERFRAMES_NO_TELEMETRY", value); + vi.resetModules(); + ({ browserTelemetryAllowed } = await import("./policy")); + expect(browserTelemetryAllowed()).toBe(false); + }, + ); + + it("ignores an unset or unrelated VITE_HYPERFRAMES_NO_TELEMETRY value", async () => { + vi.stubEnv("VITE_HYPERFRAMES_NO_TELEMETRY", "0"); + vi.resetModules(); + ({ browserTelemetryAllowed } = await import("./policy")); + expect(browserTelemetryAllowed()).toBe(true); + }); + + it("is not memoized — a mid-session opt-out takes effect immediately", () => { + expect(browserTelemetryAllowed()).toBe(true); + localStorage.setItem(DOCUMENTED_OPT_OUT, "1"); + expect(browserTelemetryAllowed()).toBe(false); + }); +}); diff --git a/packages/studio/src/telemetry/policy.ts b/packages/studio/src/telemetry/policy.ts new file mode 100644 index 0000000000..89f42ff71e --- /dev/null +++ b/packages/studio/src/telemetry/policy.ts @@ -0,0 +1,93 @@ +// --------------------------------------------------------------------------- +// Browser telemetry policy — the single answer to "may this profile be +// measured?", shared by every transport and by canary enrolment. +// +// This exists because the answer was previously duplicated and the copies had +// drifted: `telemetry/client.ts` enforced five controls, the older +// `utils/studioTelemetry.ts` transport enforced one (its own localStorage +// key), and canary evaluation enforced a different one. So a profile with +// `navigator.doNotTrack` set, or a Vite dev build, still emitted `studio:*` +// events AND could be bucketed into a rollout — under controls the public +// docs say disable both. +// +// Deliberately imports only `./config` (localStorage helpers). Nothing here +// may import a transport or the canary module: both of those import this, and +// the whole point is one definition with no cycle. +// --------------------------------------------------------------------------- + +import { isOptedOut } from "./config"; + +// Write-only PostHog project key, safe to embed in client code. Duplicated +// from client.ts intentionally — the eligibility check must not drag the +// transport (and its queue/timer state) into modules that only need the +// policy. +const POSTHOG_API_KEY = "phc_zjjbX0PnWxERXrMHhkEJWj9A9BhGVLRReICgsfTMmpx"; + +/** Legacy opt-out key predating `telemetry/config.ts`. Still honoured so + * anyone already opted out is never quietly re-enabled. */ +const LEGACY_OPT_OUT_KEY = "hf-studio-telemetry-opt-out"; + +function isLegacyOptedOut(): boolean { + try { + return localStorage.getItem(LEGACY_OPT_OUT_KEY) === "1"; + } catch { + return false; + } +} + +function isDoNotTrackOn(): boolean { + return typeof navigator !== "undefined" && navigator.doNotTrack === "1"; +} + +function isApiKeyConfigured(): boolean { + return POSTHOG_API_KEY.startsWith("phc_"); +} + +// VITE_HYPERFRAMES_NO_TELEMETRY mirrors the CLI's HYPERFRAMES_NO_TELEMETRY=1 +// opt-out so HeyGen's own dev/CI builds can suppress telemetry from the studio +// bundle the same way. Vite injects it at build time. Match the CLI's +// affirmative privacy-control spellings. +// `import.meta.env` may be undefined in non-Vite bundlers (Next.js Turbopack). +function isBuildTimeOptOut(): boolean { + try { + const v = import.meta.env.VITE_HYPERFRAMES_NO_TELEMETRY as string | undefined; + return v !== undefined && ["1", "true", "yes", "on"].includes(v.trim().toLowerCase()); + } catch { + return false; + } +} + +// `import.meta.env.DEV` is true under `vite dev` / `vite preview`. Auto-suppress +// so developers running `hyperframes preview` don't pollute production telemetry. +function isViteDevMode(): boolean { + try { + return import.meta.env.DEV === true; + } catch { + return false; + } +} + +/** + * May this browser profile be measured at all? + * + * Governs BOTH sending events and canary enrolment. Enrolment is part of + * measurement, not separate from it: a profile that reports nothing cannot be + * compared against anyone, so bucketing it changes that user's code path for + * no signal. The one documented exception is an explicit `HF_CANARY_*` / + * `?hf_canary_*=` override, which callers apply before consulting this. + * + * Not memoized — `isOptedOut()` reads localStorage, which a user can flip in + * DevTools mid-session, and the per-call cost is a couple of property reads. + * Callers that must stay stable within a session memoize their own result + * (canary decisions do; the transports intentionally do not). + */ +export function browserTelemetryAllowed(): boolean { + return ( + isApiKeyConfigured() && + !isBuildTimeOptOut() && + !isViteDevMode() && + !isOptedOut() && + !isLegacyOptedOut() && + !isDoNotTrackOn() + ); +} diff --git a/packages/studio/src/utils/studioTelemetry.test.ts b/packages/studio/src/utils/studioTelemetry.test.ts index a50b0bbb67..b7eb42cf60 100644 --- a/packages/studio/src/utils/studioTelemetry.test.ts +++ b/packages/studio/src/utils/studioTelemetry.test.ts @@ -6,18 +6,24 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; // canary work established: the documented opt-out did not silence it, and its // events carried no cohort assignment. These pin both. -const DOCUMENTED_OPT_OUT = "hyperframes-studio:telemetryDisabled"; -const LEGACY_OPT_OUT = "hf-studio-telemetry-opt-out"; - vi.mock("../telemetry/canary", () => ({ canaryEventProperties: () => ({ "$feature/canary-test-one": "true" }), })); +// One shared policy now governs this transport, telemetry/client.ts and +// canary enrolment. Exercised directly here so each case names the control +// under test rather than relying on ambient import.meta.env. +const policyState = { allowed: true }; +vi.mock("../telemetry/policy", () => ({ + browserTelemetryAllowed: () => policyState.allowed, +})); + describe("studioTelemetry — shared opt-out and canary properties", () => { let trackStudioEvent: typeof import("./studioTelemetry").trackStudioEvent; let fetchMock: ReturnType; beforeEach(async () => { + policyState.allowed = true; localStorage.clear(); vi.resetModules(); vi.useFakeTimers(); @@ -40,19 +46,15 @@ describe("studioTelemetry — shared opt-out and canary properties", () => { return parsed.batch ?? []; } - it("honours the documented opt-out key", async () => { - // Previously only the legacy key was checked, so a user who opted out the - // documented way kept emitting every `studio:*` event. - localStorage.setItem(DOCUMENTED_OPT_OUT, "1"); - trackStudioEvent("thing_happened"); - expect(await sentEvents()).toHaveLength(0); - }); - - it("still honours the legacy opt-out key", async () => { - // Anyone already opted out this way must not be quietly re-enabled. - localStorage.setItem(LEGACY_OPT_OUT, "1"); + // Every control the shared policy enforces — documented key, legacy key, + // navigator.doNotTrack, VITE_HYPERFRAMES_NO_TELEMETRY, Vite dev mode, API + // key eligibility. Before the policy was shared this transport honoured + // only the legacy key, so all of the others still emitted `studio:*`. + it("sends nothing when the shared policy refuses", async () => { + policyState.allowed = false; trackStudioEvent("thing_happened"); expect(await sentEvents()).toHaveLength(0); + expect(fetchMock).not.toHaveBeenCalled(); }); it("attaches canary assignments to every event", async () => { diff --git a/packages/studio/src/utils/studioTelemetry.ts b/packages/studio/src/utils/studioTelemetry.ts index 83b4fe2edc..373127d0ce 100644 --- a/packages/studio/src/utils/studioTelemetry.ts +++ b/packages/studio/src/utils/studioTelemetry.ts @@ -1,5 +1,5 @@ import { resolveStudioDistinctId } from "../telemetry/distinctId"; -import { isOptedOut } from "../telemetry/config"; +import { browserTelemetryAllowed } from "../telemetry/policy"; import { canaryEventProperties } from "../telemetry/canary"; // PostHog public ingest key — write-only, safe to ship in the client bundle @@ -29,21 +29,14 @@ function getDistinctId(): string { } /** - * Honours BOTH opt-out keys. - * - * This path predates telemetry/config.ts and shipped its own key, so the - * documented `hyperframes-studio:telemetryDisabled` was silently ignored here - * — `studio:*` events kept flowing for anyone who opted out the documented - * way. The legacy key stays honoured so nobody who already opted out gets - * quietly re-enabled by this fix. + * This path predates telemetry/config.ts and enforced only its own + * localStorage key, so `navigator.doNotTrack`, VITE_HYPERFRAMES_NO_TELEMETRY, + * Vite dev mode and the documented `hyperframes-studio:telemetryDisabled` all + * failed to silence `studio:*` events. Now one shared policy governs every + * transport — including the legacy key, which it still honours. */ function isEnabled(): boolean { - if (isOptedOut()) return false; - try { - return localStorage.getItem("hf-studio-telemetry-opt-out") !== "1"; - } catch { - return true; - } + return browserTelemetryAllowed(); } function getSessionProperties(): EventProperties { From 3f69a2c635f7d81f9aaf2bb0cce4e4681114d399 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Fri, 31 Jul 2026 01:27:37 -0700 Subject: [PATCH 20/20] fix(cli,core,studio): close 15 review findings + 2 R5 blockers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R5 blockers - Negative install-state latch was cached for the process lifetime, but only `true` is monotonic across processes. A long-lived preview server held a stale `false` and could re-enrol after another process tripped the breaker. Only the positive is cached now; `false` re-reads. - The real breaker writer used writeConfig(), which collapses {ok:true, mirrored:false} to success, so a run that mirrored nothing reported done with the latch only on the erasable store. It consumes writeConfigWithResult and retries until both stores carry it. Bucketing integrity - Storage-restricted Studio profiles all bucketed on the literal "anonymous": computed against the shipped hash, 100% of them were enrolled in calibration-50 rather than 50%, and they merged into one PostHog person. Per-session random id instead — persists nothing. - bucketSeed had read/write authority backwards: install-state is write-once authoritative, but readConfig took config.json's blindly, so the stores could hold different seeds until a re-mint flipped every cohort. Merged on read, like the latch. - An unwritable ~/.hyperframes with no config.json re-minted per call, re-rolling the seed on every command, and the "cohorts will not be stable" warning was unreachable on that path. - A corrupt PRE-MOVE state file was never deleted, so a machine reset with `rm -rf ~/.hyperframes` reported predecessorFound/stateFileCorrupt forever — poisoning the exact metric this work exists to produce. Opt-out honoring - CLI canary decisions memoized per process, so `hyperframes telemetry disable` during a running preview server was ignored for hours while the server kept serving pre-opt-out decisions. The memo is keyed on the telemetry posture. - shouldTrack() memoized, contradicting policy.ts's documented "not memoized" contract that policy.test.ts asserts. - The Studio override path resolved the bucket unit eagerly as an argument, minting and PERSISTING a tracking id for an opted-out profile — a value evaluateCanary discards unread. - Storage reads could throw out of telemetry into a post-commit catch block, reporting an already-committed edit as failed. - readConfig printed an unsilenceable stderr warning on every invocation for installs that opted out of telemetry entirely. Host split - isLoopbackHost rejected 0.0.0.0, so the documented HYPERFRAMES_PREVIEW_HOST LAN mode silently lost CLI→Studio identity stitching and split one user across two PostHog persons. Identity is now allowed when the operator explicitly opted into LAN binding. - Corrected the comment claiming the guard refuses spoofed Hosts: a non-browser client sets Host freely. It is a browser DNS-rebinding mitigation, not access control, and now says so. Semantics and test hygiene - percentage:100 did not mean everyone — exclude and no_unit_id sat above the fast path, so the registry's "delete the entry at 100" step was an unstaged flip for CI and seedless installs. - CLI cohort adoption returned before evaluateCanary, dropping Studio's own webdriver exclusion. - overdueCanaries() was asserted against wall-clock time, so the whole core suite would go red on 2026-09-15 for every unrelated PR; and `>` against midnight made a canary overdue ON its sunset date. - Statistical assertions ran on unseeded randomUUID() populations tight enough to fail ~1 run in 200. Seeded. Also: broke a config -> policy -> transport -> config import cycle by moving POSTHOG_API_KEY to a leaf module. Tests: 2347 CLI (bundle absent), 3153 Studio, 1450 core. Fault injection covers the latch, seed authority, LAN identity, webdriver exclusion and the anonymous-bucket fix. Two pre-existing tests asserted behaviour these findings identify as wrong (shouldTrack memoization, 100%-excludes-CI) and were rewritten with the reasoning stated. Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/commands/render.test.ts | 47 +++++ packages/cli/src/commands/render.ts | 17 +- packages/cli/src/commands/telemetry.test.ts | 4 +- packages/cli/src/server/studioServer.ts | 4 +- .../cli/src/server/telemetryIdentity.test.ts | 49 ++++- packages/cli/src/server/telemetryIdentity.ts | 37 +++- packages/cli/src/telemetry/canary.test.ts | 49 +++-- packages/cli/src/telemetry/canary.ts | 14 ++ packages/cli/src/telemetry/config.test.ts | 125 +++++++++++++ packages/cli/src/telemetry/config.ts | 173 +++++++++++++----- packages/cli/src/telemetry/policy.test.ts | 4 +- packages/cli/src/telemetry/policy.ts | 2 +- packages/cli/src/telemetry/posthogKey.ts | 10 + packages/cli/src/telemetry/transport.ts | 3 +- packages/core/src/canary.test.ts | 89 +++++++-- packages/core/src/canary.ts | 10 +- packages/core/src/canaryRegistry.ts | 7 +- packages/studio/src/telemetry/canary.ts | 13 +- packages/studio/src/telemetry/client.test.ts | 11 +- packages/studio/src/telemetry/client.ts | 12 +- packages/studio/src/telemetry/config.ts | 18 +- .../studio/src/telemetry/distinctId.test.ts | 35 ++++ packages/studio/src/telemetry/distinctId.ts | 13 +- packages/studio/src/telemetry/policy.ts | 11 ++ 24 files changed, 648 insertions(+), 109 deletions(-) create mode 100644 packages/cli/src/telemetry/posthogKey.ts diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index 092860ce30..bbfc4b55c1 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 d832746369..78fbd65674 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 3fe6150328..27e62fbc7f 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 e026679c41..8b5e7c10ab 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 83ea864c1e..169f9ceb27 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 f8d027274c..a518a87083 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 80582db91f..4771c0c145 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 3026f8034e..f14de2e5b7 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 09b42ff46a..a5e7674409 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 30eb92451c..df587663ef 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 4894f29fcf..4ffc93f290 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 78e97df84a..8daa7cfefb 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 0000000000..fe72aa24ae --- /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 9ec9ac0031..42fe4971cd 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 f80cb2fa28..03da853ca2 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 cf8d40fb41..5fb5cd6f31 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 d74a2b1394..f0ad7936c4 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 8c90f8088c..d6240b2b78 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 079bbe4966..85602bb74b 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 511571bb47..615d497d31 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 f4326b97b0..970f5b136f 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 b420114639..b6e77119be 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 241b9225ac..790beaf888 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 89f42ff71e..9cf123dae5 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() &&