feat(core): percentage-based canary rollouts + calibration experiment - #2854
feat(core): percentage-based canary rollouts + calibration experiment#2854vanceingalls wants to merge 20 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
miga-heygen
left a comment
There was a problem hiding this comment.
SSOT -- single source of truth audit
Clean across the board.
One canonical bucketing function. evaluateCanary in packages/core/src/canary.ts is the single decision maker. Both CLI and Studio bindings import it from @hyperframes/core/canary via the leaf subpath -- neither duplicates the hash, the comparison, or the clamping logic.
One registry. CANARIES in packages/core/src/canaryRegistry.ts is the sole rollout table. Both bindings import findCanary and CANARIES from @hyperframes/core/canary-registry. Percentage lives in the registry, never at the call site.
Symmetric bindings. CLI and Studio expose the same public API (isCanaryEnabled, resolveCanary, canaryEventProperties) with the same signatures and semantics. The three surface-specific inputs (unit id, override mechanism, exclusion signal) differ only in how they're sourced:
| Input | CLI | Studio |
|---|---|---|
| Unit id | config.bucketSeed ?? config.anonymousId |
window.__HF_CLI_BUCKET_SEED ?? resolveStudioDistinctId() |
| Override | HF_CANARY_<FEATURE> env var |
?hf_canary_<name>=on query param + sessionStorage |
| Exclusion | getSystemMeta().is_ci |
navigator.webdriver |
Both memoize decisions for the lifetime of the process/page load. Both use parseCanaryOverride for the same on/off/true/false/1/0/yes/no vocabulary. Override always wins over percentage in both directions on both surfaces. Both emit both arms in telemetry ("false" = control, absent = build predates canary).
State file is the single owner of machine-lineage. ~/.local/state/hyperframes/install-state.json carries exactly three fields: markerAt, deParallelRouterTrialFired, bucketSeed. Written only through syncInstallState, called exclusively from writeConfig. No identity leaks into this file (asserted by test: file keys are exactly those three, and bucketSeed !== anonymousId).
No SSOT findings.
Correctness
FNV-1a implementation verified. The multiply-by-shift trick decomposes hash * 16777619 (the FNV-1a 32-bit prime) as hash + (hash<<1) + (hash<<4) + (hash<<7) + (hash<<8) + (hash<<24), which equals hash * (1 + 2 + 16 + 128 + 256 + 16777216) = hash * 16777619. I independently confirmed all five test vectors ("" -> 0x811c9dc5, "a" -> 0xe40c292c, "b" -> 0xe70c2de5, "foobar" -> 0xbf9cf968, "hello" -> 0x4f9f2cab) match canonical FNV-1a 32-bit output. The test suite also cross-checks the shipped canaryBucket against a local FNV-1a copy on 200 UUIDs x 3 features -- that cross-check is what prevents the vector test from being tautological.
Bucket comparison is correct. bucket < pct with bucket in [0, 99]:
- 0% ->
pct = 0-> early returnpct <= 0-> off for everyone. - 100% ->
pct = 100-> early returnpct >= 100-> on for everyone. - 10% -> buckets 0-9 enrolled (exactly 10 of 100). Using
<=would enroll 11. - Fractional percentages truncated (10.9 -> 10), tested explicitly.
- Out-of-range clamped (negative -> 0, >100 -> 100), tested.
Inclusive ramping. bucket < 10 is a proper subset of bucket < 25 -- widening never drops an enrolled install. Tested on 500 UUIDs.
Independence per feature. Hashing feature:unitId instead of unitId alone. Tested: 8 concurrent 10% canaries on 20k UUIDs show binomial(8, 0.1) distribution (43% in none, 0 in all 8 -- correlated slices would put ~10% in all). Plus overlap between feat-a and feat-b at 10% is <70/2000, not ~200 (which would indicate identical slices).
Uniformity. Chi-square on 30k UUIDs across 100 buckets: asserted < 148.2 (critical at p=0.001, df=99). Plus accuracy within 1pp at n=20k for percentages 1/5/10/25/50.
Memoization. Both bindings memoize in a Map<string, CanaryDecision>. A late env change / URL param change after initial resolution does NOT flip the decision -- tested explicitly. Registry changes require a process/page restart anyway (new package version). This is correct and intentional.
No correctness findings.
State management
Write-once bucketSeed. In nextInstallState: bucketSeed: state?.bucketSeed ?? config.bucketSeed -- an existing lineage seed always wins. Tested: "a later install never overwrites the lineage seed" writes config multiple times and verifies the state file seed stays anchored to the first install.
Atomic writes. Both writeConfig and writeInstallState use the tmp+rename pattern (${path}.${process.pid}.tmp -> rename). Correct for crash safety.
Breaker carryover. latchedFired returns true if either the state file or the config has it tripped (latching OR). mintConfig reads state and carries deParallelRouterTrialFired forward. Tested: survives config wipe, survives config corruption, does NOT invent an untripped breaker.
No identity in state file. Test asserts Object.keys(stateFile()).sort() === ["bucketSeed", "deParallelRouterTrialFired", "markerAt"]. Seed asserted distinct from anonymousId. JSON.stringify(stateFile()) asserted to NOT contain the anonymousId.
Legacy backfill. Configs predating bucketSeed get a one-time backfill (preferring the lineage seed from the state file if one exists, else minting a fresh UUID). Persisted immediately so the seed doesn't re-roll per readConfigFresh. Tested: backfill is stable across fresh reads, and a legacy config adopts the lineage seed.
State-write failure isolation. syncInstallState swallows errors and leaves sync memos unset so a later write retries. The writeConfig return value reflects only the config write, not the state sync. Tested: a state-file write failure (EACCES) does not break the config write.
No state management findings.
Security / Privacy
bucketSeed never emitted in telemetry. The CLI telemetry client spreads only canaryEventProperties() (which returns $feature/canary-<name>: "true"/"false" for each registered canary) and install_predecessor_found (boolean). The seed is passed locally from CLI to Studio via window.__HF_CLI_BUCKET_SEED and /api/telemetry-identity, but these are localhost-only paths. Verified: no code path attaches bucketSeed to a PostHog event.
Flag vector as cross-wipe linker. The PR acknowledges this explicitly in the docs: with N canaries, the k-bit assignment vector is a low-entropy linker. At 3 canaries (current), that's 8 possible vectors -- not individually identifying. The calibration docs note this as the residual privacy cost, traded against cumulative-exposure-drift reduction. Acceptable.
XSS hardening on injected values. encodeInlineScriptValue escapes < and / in the JSON-stringified seed before injecting into the inline <script> tag, preventing </script> injection. Belt-and-suspenders since the values are randomUUID().
No security findings.
Standards
No bare as T in production code. The only production-side casts are (1) JSON.parse() as Partial<InstallState> followed by shape validation in readInstallState, and (2) as RecentRenderRecord inside a type-guard filter in parseRecentRenders. Both are the standard defensive patterns. All other as casts are in test files.
No non-null assertions. None found in the diff.
Package subpaths properly configured. ./canary and ./canary-registry added to package.json (development exports with bun/node/import/types conditions, publishConfig exports with import/types), and to package-subpaths.json (source/runtime/types/environments). Pattern matches the existing ./beats entry. Both subpaths list all three target environments (browser, bun, node).
Barrel re-exports. packages/core/src/index.ts re-exports both modules for consumers that don't use subpaths. The CLI and Studio bindings import from the subpaths (not the barrel), which is correct -- the barrel pulls the whole core surface and would bloat the Studio browser bundle.
No standards findings.
Side-effect invariant
Pure evaluator. evaluateCanary takes all inputs as arguments, does no I/O, reads no global state, writes nothing. Browser-safe by construction.
State writes only in the writeConfig path. syncInstallState is called exclusively from writeConfig. The readConfig path triggers writeConfig only during mint (first run or corrupted config) -- which is correct, since a fresh install needs to create the state file.
Both arms emitted in telemetry. canaryFeatureProperties iterates over ALL registered canaries, mapping enrolled to "true" and control to "false". Absent means "this build predates the canary" -- a different fact from "not enrolled". Tested on both CLI and Studio.
No side-effect findings.
CI
Both required checks pass:
- WIP: completed, success
- Mintlify Deployment: completed, success
Nits
-
Orphaned JSDoc in
telemetryIdentity.ts. The insertion ofresolveCliBucketSeedandencodeInlineScriptValuebetween the old JSDoc forbuildCliIdentityScriptand the function itself meansbuildCliIdentityScriptno longer has its own JSDoc directly above it. The old comment ("Empty string when there's nothing to seed") now floats aboveresolveCliBucketSeed. Worth moving it back down or replacing it. -
CanaryReasonJSDoc says "Attach to telemetry" but the reason is deliberately not attached (only the boolean assignment is). The calibration docs say "Emitting a reason property is deliberately skipped -- add it only if this check comes back dirty." The type-level comment could mislead a future reader into thinking the reason IS on every event. Consider softening to "Available for telemetry if segmentation by reason is ever needed." -
as conston theCANARIESarray is redundant with thereadonly CanaryDefinition[]type annotation (the annotation already prevents mutations and widens the element types back toCanaryDefinition). Harmless, but if the intent is deep immutability,satisfieswould be clearer.
Verdict
Approve. This is well-designed infrastructure with an unusually strong test suite (chi-square uniformity, binomial independence cross-check, FNV vector verification, breaker corruption survival, write-once lineage seed). The SSOT is clean, the fail-closed invariant is consistent across both surfaces, the state-file separation is the right call for breaker carryover, and the calibration experiment is a thoughtful "measure before you depend on it" approach.
The three nits above are readability touches, none affect correctness or safety.
-- Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed exact head a2f2827f421b529bf33c52317d39c3305da6f1fd. The canary evaluator/registry/bindings and machine-lineage state separation are coherent, and Miga’s invariant review matches the implementation I sampled. I am withholding the approval stamp at this head for one procedural blocker: GitHub reports mergeStateStatus=DIRTY, and the only checks attached are WIP + Mintlify—not the repository CI lanes. This is a large rollout/telemetry/state change, so it needs a clean rebase onto current main and fresh build, typecheck, lint, tests, Fallow, runtime/CLI/Studio contract checks before approval. Ping me on the rebased exact head and I will delta-review/stamp it.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at a2f2827.
Cross-cutting take: the design is a big lift and it reads as thoughtful — the pure evaluator, the registry with sunset guard, the state-file lineage, the "both arms emitted" flag-property semantics, the cumulative-exposure analysis in the docs, and the pre-registered calibration checks all read as a well-considered rollout mechanism rather than a percentage-flag with docs stapled on. docs/contributing/canary-rollouts.mdx is the clearest primitive-doc I've seen land in HF and I'd trust it as spec.
Concerns below are about specific places where the code diverges from the docs' own invariants, or where the state carryover has recovery paths that quietly break the "cohorts survive" claim under adverse conditions. Inline for each concrete site.
Highest-priority
-
studio:*events do not carry canary properties. Onlystudio_*events (viapackages/studio/src/telemetry/client.ts— which IS in this diff) got the canary wiring; the siblingpackages/studio/src/utils/studioTelemetry.ts(session_start,render_start,crash,commit_transaction,panel_toggle,tab_switch, ~30 call sites) does NOT — it never importscanaryEventProperties.docs/contributing/canary-rollouts.mdx:60claims "Every telemetry event carries the assignment." Calibration check 4 (cross-surface agreement) will be biased on thestudio:*stream, and any Studio-consumer canary added later will silently miss half its Studio events. Not attaching an inline because the file isn't in the diff — this is a missing wiring rather than a bad line — but the concrete fix mirrorsclient.ts's import + merge into the batch payload. -
CLI
HF_CANARY_<FEATURE>env override does not propagate to a CLI-launched Studio. See inline ontelemetryIdentity.ts:70. The docs sell the env override as the primary escalation channel; in practice CLI-only override + Studio-percentage means the two surfaces silently diverge for the exact use case (support / dogfooding / panic-off) the mechanism was built to serve. -
Cohort divergence when CLI telemetry is off. See inline on
telemetryIdentity.ts:54. A canary is a behavioral switch, not just a reporting field — CLI still buckets onconfig.bucketSeedwhen telemetry is disabled, but Studio never receives the seed and falls back to its own distinctId. Two surfaces, same install, different code paths.
Concerns worth resolving before ramp
/api/telemetry-identityreturns the raw bucket seed to any localhost caller with no origin check (inline).readInstallStatereturnsnullon any partial corruption, discarding a salvageablebucketSeed(inline).predecessorFound: state !== nullconflates "no predecessor" with "corrupt state file", polluting the recoverable-churn metric the feature exists to measure (inline).- Legacy backfill ignores
writeConfig's return value; on unwritable HOME the machine mints a fresh seed per process invocation with no diagnostic (inline). autoUpdate.ts(not in this diff) launches a detached child that read-modify-writes~/.hyperframes/config.jsondirectly, bypassingwriteConfig. If the main process tripsdeParallelRouterTrialFired: truewhile the child holds a stale snapshot, the child's rename silently drops the tripped fact — and because the config parse path never re-hydrates from the state file (only the mint path does), the tripped fact is gone until the trial fails again. Directly contradicts the PR body's "sync lives insidewriteConfigso no breaker write site can forget it."
Nits / follow-ons
- FNV-1a hashes UTF-16 code units, not UTF-8 bytes (inline).
- No test pins
de-parallel-routerat 0% (inline on the registry). - Test flake risk at pct=50: 1pp band ≈ 2.83σ at n=20000 = ~0.4% baseline flake; the chi-square is asserted against p=0.001 = 0.1% baseline flake. UUIDs are unseeded (
randomUUIDper run), so re-runs draw independently. Not blocking, but consider seeding the UUID population for these two tests to remove the noise. - Barrel at
packages/core/src/index.ts:333-340re-exportscanaryBucket/evaluateCanary/parseCanaryOverride+ types but silently dropscanaryFeatureKey/canaryFeatureProperties/CANARY_FEATURE_PREFIX. Consumers use leaf imports so nothing breaks today — cleanup nit. - Studio URL-param lookup is lowercase-only (
canaryParamNameforce-lowercases inpackages/studio/src/telemetry/canary.ts:47-48).?HF_CANARY_DE_PARALLEL_ROUTER=on(upper-snake, matching CLI env-var muscle memory) silently no-ops. Docs are lowercase; a case-insensitive read or an assertion at usage would surface the mistake. ?hf_canary_x=resetonly takes effect on cold cache (resolveCanaryreturns memoized decision). A page reload works around; docs could clarify.
CI + merge state
mergeStateStatus: DIRTY— merge conflict againstorigin/main(37 ahead / 13 behind).- CI is largely un-run on this branch — only Mintlify Deployment + WIP fired. The GitHub Actions checks (Typecheck / Lint / Format / regression-shards / Test / Preflight / etc.) that gate every other PR in this repo have not reported. Worth confirming what triggered the CI silence — the PR won't be mergeable until it's rebased + CI reads green.
What I didn't verify
- I read the pure evaluator, both bindings,
config.ts+telemetryIdentity.ts+studioServer.ts+client.ts+ relevant tests; I did not exercise the CLI end-to-end. - I dispatched 4 sub-agents in parallel for the CLI binding, Studio binding, core evaluator, and config carryover — each was asked to quote file:line for every finding. Findings above have been spot-verified against the actual code before landing here.
| */ | ||
| export function resolveCliBucketSeed(): string | null { | ||
| try { | ||
| if (!telemetryShouldTrack()) return null; |
There was a problem hiding this comment.
🟠 Seed injection gated on telemetry-tracking, but CLI canary evaluation is not. telemetryShouldTrack() is a reporting gate — the CLI still evaluates canaries (which control code paths, e.g. process.env.HF_DE_PARALLEL_ROUTER = "true" in render.ts) regardless of telemetry state, bucketing on config.bucketSeed.
When telemetry is off (HYPERFRAMES_NO_TELEMETRY=1 / DO_NOT_TRACK / CI), resolveCliBucketSeed() returns null → buildCliIdentityScript() skips the __HF_CLI_BUCKET_SEED line entirely → Studio has no CLI seed to adopt → Studio falls back to resolveStudioDistinctId(). Same install, two different unit ids, potentially two different cohorts, two different code paths on the same render. Directly breaks canary-rollouts.mdx:203-205 ("the bucketing unit … shared with a CLI-launched Studio so both surfaces agree") and calibration check 4.
The gate should probably be "is the seed safe to expose" (which it is by design — a random UUID with no PII, no identity linkage per docs), not "is telemetry on". — Rames D Jusso
There was a problem hiding this comment.
Fixed in 4f464dc4. You and @jrusso1020 landed on this independently, which is what moved it to the top of the list.
You had the asymmetry exactly right: telemetryShouldTrack() gated seed injection while evaluation ran regardless, so an opted-out install was still bucketed and still had HF_DE_PARALLEL_ROUTER flipped underneath it.
The fix closes it at evaluation rather than at injection — resolveCanary returns a new telemetry_opt_out reason before evaluateCanary is ever called, so there's no bucket to inject a seed for. Same rule mirrored in Studio against hyperframes-studio:telemetryDisabled.
One implementation note, since it looks like an obvious refactor otherwise: the CLI check mirrors shouldTrack() rather than importing it. client.ts already imports canary.ts for canaryEventProperties, so depending on it here would be an import cycle. Both read the same two inputs — policy.ts's runtime override, then the persisted preference — so they can't drift apart; that's noted at the call site.
| return `<script>window.__HF_CLI_DISTINCT_ID=${encoded};</script>`; | ||
| const parts = [`window.__HF_CLI_DISTINCT_ID=${encodeInlineScriptValue(cliId)};`]; | ||
| const seed = resolveCliBucketSeed(); | ||
| if (seed) parts.push(`window.__HF_CLI_BUCKET_SEED=${encodeInlineScriptValue(seed)};`); |
There was a problem hiding this comment.
🟠 HF_CANARY_<FEATURE> env override does not propagate to CLI-launched Studio. This script injects __HF_CLI_DISTINCT_ID + __HF_CLI_BUCKET_SEED but NEVER process.env.HF_CANARY_*. Studio reads overrides only from window.location.search / sessionStorage (packages/studio/src/telemetry/canary.ts:98-119).
A support session running HF_CANARY_DE_PARALLEL_ROUTER=on hyperframes preview gets: CLI → forced_on (correct); Studio → whatever the percentage says for that seed (silently wrong). The docs (canary-rollouts.mdx:54-56) sell overrides as the escalation channel — dogfooding, bisects, panic-off — and this is exactly where the two surfaces should agree.
Cheapest fix: forward the resolved override decision (not the raw env-var) via a per-feature injected map, e.g. window.__HF_CLI_CANARY_OVERRIDES = { "de-parallel-router": true } — resolved once by the CLI, adopted by the Studio binding, wins over URL param + registry percentage on both sides. — Rames D Jusso
There was a problem hiding this comment.
Fixed in 98b23a88, using your suggested shape — generalised slightly to cover the sibling finding at the same time.
The CLI now publishes window.__HF_CLI_CANARY_DECISIONS ({ name: boolean }) and Studio takes it as authoritative over its own seed, URL override and registry percentage. Injecting the resolved decision rather than the raw env var is what you proposed; extending it from overrides to all decisions also closes 3670330763, because the same re-derivation was the root of both.
Worth flagging that 3670330763 was not fully closed by the telemetry-opt-out commit before it, as I'd initially thought. That gated each surface independently — which stopped silent enrolment per surface but left exactly the divergence you described: DO_NOT_TRACK=1 hyperframes preview gave CLI → telemetry_opt_out, Studio → separate localStorage flag unset → evaluates on its fallback id → may enrol. Publishing decisions is what actually closes it, since the map is emitted even when telemetry is off — that's the case it exists for.
On your framing ("the gate should be is the seed safe to expose, not is telemetry on"): agreed in spirit, and the decisions map sidesteps it — booleans about features are safe to publish where the seed isn't, and Studio no longer needs the seed to agree with the CLI. That also shrinks the surface behind 3670330769; the endpoint may not need to serve bucketSeed at all.
Studio still evaluates locally when standalone or for a canary the CLI didn't publish, and ignores a non-boolean value rather than trusting it. Tests: 6 Studio, 4 CLI (including script-tag escaping on a hostile canary name and a throwing resolver degrading to identity-only). Fault injection: dropping the adoption fails 4.
| return c.json({ distinctId: resolveCliTelemetryDistinctId() }); | ||
| return c.json({ | ||
| distinctId: resolveCliTelemetryDistinctId(), | ||
| bucketSeed: resolveCliBucketSeed(), |
There was a problem hiding this comment.
🟡 Unauthenticated endpoint exposes the raw bucketSeed to any localhost caller. No Origin/Referer/Host check, no CORS gate.
Standard SOP blocks a cross-origin fetch() from reading the JSON body, but:
- Any other local process on the machine can GET this endpoint and read the anonymous distinct id + raw seed at will — a strictly worse privacy surface than the inline
<script>injection attelemetryIdentity.ts:70-77, which is scoped to Studio's own document. - DNS-rebinding: a remote page can rebind a hostname to
127.0.0.1and read the response as same-origin. The seed then leaks and can be joined to any prior PostHog events sharing that distinct id.
Docs comment above the handler calls this a "fallback for clients that can't rely on the injected global" but doesn't name the trade-off. Cheapest close: require a Host: localhost (or the exact bound host) check via c.req.header("host"), and/or serve the endpoint only when req.headers.get("origin") matches the studio's own origin. — Rames D Jusso
There was a problem hiding this comment.
Fixed in 31361b8e5 — closed by removal rather than a guard.
The endpoint no longer serves bucketSeed at all. Now that Studio adopts window.__HF_CLI_CANARY_DECISIONS, nothing needed the seed over HTTP, so the field just went away — and you were right that an unauthenticated local endpoint is a strictly worse surface than the inline script scoped to Studio's own document.
The endpoint itself predates this PR and still serves distinctId, so it also gained the Host guard you suggested: isLoopbackHost() accepts localhost, the whole 127.0.0.0/8 block and [::1] (with or without port), and refuses everything else. That's what makes DNS rebinding catchable — the rebound request still arrives carrying the attacker's hostname. 17 cases pinned, including 127.0.0.1.evil.com and localhost.evil.com.
| try { | ||
| if (!existsSync(STATE_FILE)) return null; | ||
| const parsed = JSON.parse(readFileSync(STATE_FILE, "utf-8")) as Partial<InstallState>; | ||
| if (typeof parsed.markerAt !== "string") return null; |
There was a problem hiding this comment.
🟡 Any partial corruption of the state file drops bucketSeed even when it was salvageable. The nuclear-drop on missing markerAt at line 57 discards the whole record — so if a hand-edit or partial disk write mangles markerAt while leaving bucketSeed intact, the next mintConfig mints a fresh seed and writeConfig → syncInstallState overwrites the recoverable seed with the new one. Cohort lineage lost, no diagnostic.
The docstring at line 84-85 acknowledges the trade-off ("a corrupted state file silently reads as absent"), but the choice sacrifices the invariant the PR body foregrounds ("cohorts survive the wipe"). Suggested: if markerAt is malformed, still attempt bucketSeed recovery — a valid UUID-shaped string is the higher-value fact to preserve, and markerAt can be regenerated. The corruption case is rare, but the recovery path only has to be right for a small minority to matter for fleet-level cohort math. — Rames D Jusso
There was a problem hiding this comment.
Fixed in 31361b8e5. Took your suggestion directly: a malformed markerAt no longer discards the record.
markerAt is only a timestamp and gets restamped; bucketSeed is unrecoverable, and losing it silently re-rolls the install's cohort — so the seed is the fact worth preserving, exactly as you argued. Only when neither field survives is the record treated as corrupt.
Also updated the docstring you quoted: it claimed a corrupt file 'silently reads as absent', which is no longer true and was the thing making the trade-off invisible.
| return { | ||
| ...DEFAULT_CONFIG, | ||
| anonymousId: randomUUID(), | ||
| predecessorFound: state !== null, |
There was a problem hiding this comment.
🟡 predecessorFound conflates "no predecessor" with "corrupt state file". readInstallState returns null for BOTH "file absent" AND "file present but unparseable" (line 66 catch, plus the markerAt drop at line 57). So the mint reports predecessorFound: false in the corruption case, even though the machine did have a previous install.
Per the PR body, install_predecessor_found is the metric that "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)." Silently emitting false for corruption events biases that split — a machine with a partial disk write looks like a genuinely fresh mint, understating recoverable churn and overstating fresh mints.
Cheapest fix: distinguish "absent" (fs stat says no file) from "corrupt" (file exists but parse failed) in readInstallState, and emit predecessorFound: "absent" | "corrupt" | true — or a boolean plus a separate stateFileCorrupt counter for the residual. — Rames D Jusso
There was a problem hiding this comment.
Fixed in 31361b8e5 — and this one mattered more than its 🟡, because it biases the exact number the rollout gets judged on.
readInstallState now distinguishes absent from corrupt, predecessorFound reports true for corruption (the machine did have an install), and install_state_file_corrupt ships alongside so the true share can be split rather than just inflated.
Went with your boolean-plus-counter option over the tri-state string: it keeps the existing property's type stable for anything already querying it, and the residual is what you actually want to filter on.
Worth noting the field currently reads false on 155k/155k events, so it has no measured signal yet either way — which is precisely why I didn't want it silently under-reporting once it does.
| // Persisted immediately — an unpersisted seed would re-roll every process. | ||
| if (config.bucketSeed === undefined) { | ||
| config.bucketSeed = readInstallState()?.bucketSeed ?? randomUUID(); | ||
| writeConfig(config); |
There was a problem hiding this comment.
🟡 writeConfig's return value is ignored — a failed backfill leaves the process running on a seed that will re-roll next invocation. If ~/.hyperframes is unwritable (root-owned after a sudo mishap, read-only bind mount, disk full), writeConfig(config) returns false, syncInstallState never runs (both writes are in the same try), and the state file has no seed to inherit from. The in-process fallback (line 384: cachedConfig = config) papers over the failure for the life of this process — but the next process invocation re-enters this branch, readInstallState() returns null again, and randomUUID() mints a different seed. No log, no diagnostic — the install silently churns cohorts across invocations.
The field's own docstring at line 255-258 claims "backfilled once for configs predating the field" — true only when the write actually lands. The failure mode is silent by design, but the docs claim doesn't match. Suggested: at minimum, log to the transport (like the swallowed telemetry-write failures), and consider surfacing via hyperframes telemetry doctor. — Rames D Jusso
There was a problem hiding this comment.
Fixed in 31361b8e5. The backfill now uses writeConfigWithResult and warns once per process with the underlying error, rather than dropping the boolean.
You were right that the docstring was the tell — 'backfilled once' was only true when the write landed, and the failure mode was silent by construction. The warning names the consequence ('canary cohort assignment will not be stable across runs') rather than just the write failure, since that's the part a user can act on.
Once per process, not per call: the backfill re-runs on every readConfig until it succeeds, so an unwritable home directory would otherwise print on every command.
| function fnv1a32(input: string): number { | ||
| let hash = 0x811c9dc5; | ||
| for (let i = 0; i < input.length; i++) { | ||
| hash ^= input.charCodeAt(i); |
There was a problem hiding this comment.
🟢 FNV-1a hashes UTF-16 code units, not UTF-8 bytes. charCodeAt(i) yields a 16-bit code unit (up to 0xFFFF), two surrogate halves for astral code points. Reference FNV-1a is byte-oriented; the two agree only for ASCII.
All current inputs (kebab-case canary names, v4 UUIDs) are ASCII, so this is not a live bug — the canonical vectors at canary.test.ts:41-50 pin the ASCII case correctly. But: any future canary name that grows a non-ASCII character (an accented owner tag, an emoji, a full-width dash typed by autocorrect) will silently diverge from every other FNV-1a implementation, and any external tool that hashes canary cohorts (fleet analytics, doctor, debug tooling) won't agree.
Two ways to close: (a) require ASCII in the registry (already true by convention — a name-shape assertion on the registry test would lock it), or (b) hash the UTF-8 encoding (TextEncoder in browser, Buffer.from(s, 'utf8') in Node — but that pulls in Buffer, breaking the browser-bundle constraint). (a) is cheaper. — Rames D Jusso
There was a problem hiding this comment.
Went with your option (a), in 31361b8e5 — with the note that the guard already existed and was simply undocumented.
The registry test's kebab-case assertion (/^[a-z0-9]+(-[a-z0-9]+)*$/) already made non-ASCII names unreachable; nothing linked it to the hash, so anyone loosening it would have had no idea it was load-bearing. Now both ends say so: the test is renamed 'has unique, ASCII kebab-case names — the hash depends on this' with an explicit range check, and fnv1a32 documents that it is ASCII-only by contract, that both inputs are constrained (names by the registry, units by being UUIDs), and that widening either means switching to UTF-8 encoding first.
Agreed on (b) being the wrong trade — TextEncoder would be fine but Buffer breaks the browser-bundle constraint that motivated the six-line hash in the first place.
| // ── Real rollouts ──────────────────────────────────────────────────────── | ||
| { | ||
| name: "de-parallel-router", | ||
| percentage: 0, |
There was a problem hiding this comment.
🟢 No test pins de-parallel-router at percentage: 0. The registry-shape tests at canary.test.ts:231-239 check bounds (percentage in [0..100]) and name uniqueness, but no per-name assertion. The description here explicitly says "Ramp only alongside the per-install circuit breaker" — a patch that bumps this to 5 without also landing the breaker wiring lands green.
If the intent is "stays at 0 until further notice", a targeted assertion —
expect(findCanary("de-parallel-router")?.percentage).toBe(0);with a comment linking to the breaker-wiring PR — would guard the invariant against a hurried ramp. — Rames D Jusso
There was a problem hiding this comment.
Added in 31361b8e5, essentially your snippet:
it("keeps de-parallel-router at 0% until the circuit breaker is wired", () => {
expect(findCanary("de-parallel-router")?.percentage).toBe(0);
});With a comment making the reasoning explicit — the registry is data, so a ramp is a one-line edit with no code-review surface, and the canary's own description says to ramp only alongside the breaker. The test name states the unblocking condition so whoever hits it knows what to land first rather than just deleting the assertion.
| // Install-state file: ~/.local/state/hyperframes/install-state.json | ||
| // | ||
| // A second, deliberately separate location from CONFIG_DIR, so it survives | ||
| // the most common identity reset — deleting or reinstalling ~/.hyperframes. | ||
| // It exists to carry exactly two facts across that reset, and nothing else: |
There was a problem hiding this comment.
if someone is eleting their hyperframes config I think it should wipe all hyperframes state
i'm not sure we should try and persist state elsewhere to get around this
There was a problem hiding this comment.
Agreed, and done — this is now the design in both this PR and #2904 (merged).
install-state.json moved into ~/.hyperframes alongside config.json, so rm -rf ~/.hyperframes really is a full reset. Nothing hyperframes-related persists outside that directory, and a pre-move file is migrated in and deleted so nobody is left with a stale copy at the old path.
Your instinct was also right on the data: install_predecessor_found is false on all ~155k events since the marker shipped, so the config-wipe carryover has never once fired. (Two days is too short to prove wipes are rare, but it's certainly not evidence they're common.)
The file still earns its place, just against a different threat — config.json is rewritten on every command and every render, and readConfig re-mints a fresh identity on any parse/permission/IO failure. That re-mint is the churn that actually happens, and it's what would reshuffle canary cohorts mid-rollout. So: config re-mint → cohorts hold; directory deleted → cohorts go too.
Docs now carry an explicit removal-path section, and hyperframes telemetry status prints both paths.
| /** ISO timestamp of when the marker was first written. */ | ||
| markerAt: string; | ||
| /** Rolled-over circuit-breaker state — see HyperframesConfig's field. */ | ||
| deParallelRouterTrialFired?: boolean; |
There was a problem hiding this comment.
should this be in install state?
maybe the name is confusing but i feel like we would want their to be "install state" then per feature flag values separate from each other?
There was a problem hiding this comment.
Fair — the naming was doing two jobs. install-state.json now holds three facts and the header enumerates them explicitly rather than calling itself generic "install state":
markerAt— a previous install existed here (measures re-mint churn)deParallelRouterTrialFired— circuit-breaker tripped bitbucketSeed— canary bucketing unit
On splitting per-feature values into their own records: I'd rather not yet. There's exactly one canary in the registry (de-parallel-router, at 0%), so a per-feature store would be one file with one field and no second consumer to shape it. The seed is also deliberately not per-feature — bucketing hashes feature:seed, so one seed yields independent slices per feature and a per-feature seed would actually break that property.
If a second canary needs its own persisted state beyond the shared seed, that's the point to split — happy to revisit then.
| * 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. |
There was a problem hiding this comment.
are these FFs are configuration values for optional long lived opt ins?
i think it may be conflating the two?
There was a problem hiding this comment.
Wired up in 4f464dc4 — telemetry opt-out is now canary opt-out, on both the CLI and Studio.
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. It resolves to a new telemetry_opt_out reason before bucketing, so an opted-out install isn't assigned a cohort at all (there's a test asserting bucket stays undefined — a bucket number would mean we hashed them into a slice anyway).
An explicit HF_CANARY_<FEATURE> / ?hf_canary_<feature>= override still wins. That's a deliberate local choice rather than silent enrolment, and it stays the documented way to exercise a canary with telemetry off.
On your other question in this thread — whether these are feature flags or long-lived opt-in config — I think the gate makes the distinction concrete rather than papering over it. A canary is a measured rollout: we enrol a slice specifically so it can be compared to everyone else, which is why an install that reports nothing has no business being in one. A long-lived opt-in would be the opposite: it should work regardless of telemetry, because its value is to the user, not to us. The registry today holds only the former (de-parallel-router, at 0%). If we ever want the latter, I'd argue it belongs in config, not here — happy to make that split explicit in the docs if you'd like it stated outright.
Fault injection: removing the CLI gate fails 6 tests, removing the Studio gate fails 3.
| --- | ||
| 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. | ||
|
|
There was a problem hiding this comment.
should we add this into/connected to the existing telemetry doc?
we shoudl also make sure if a user opts out of telemetry they opt out of this
There was a problem hiding this comment.
Both halves now done — I'd only addressed the second one earlier, thanks for re-surfacing it.
Opt-out (4f464dc4): telemetry opt-out is canary opt-out, on CLI and Studio. Resolves to a telemetry_opt_out reason before bucketing, so an opted-out install isn't assigned a cohort at all — there's a test asserting bucket stays undefined. Covers the persisted preference, HYPERFRAMES_NO_TELEMETRY, DO_NOT_TRACK, dev builds, and Studio's hyperframes-studio:telemetryDisabled. An explicit HF_CANARY_* override still wins, since that's a deliberate local choice rather than silent enrolment.
Docs connection (54534d53c): there's no standalone telemetry page — the canonical disclosure is the telemetry command section in packages/cli — so I linked all three surfaces both ways rather than adding a fourth:
packages/cli#telemetry → telemetry state also controls canary enrolment, every opt-out route named, links here.- this page → a
<Note>at the very top, above any of the how-to, stating canaries sit behind the telemetry switch and why: a canary is a measured rollout, and an install that reports nothing can't be compared against anyone, so enrolling it buys no signal and only changes that user's code path. guides/feedback"Opting Out" → disabling telemetry also ends canary enrolment, listed with the feedback prompt and usage tracking.
The framing throughout is your point back to me: this isn't a separate system with its own opt-out, it's part of telemetry.
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_<FEATURE> 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) <noreply@anthropic.com>
…cument 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) <noreply@anthropic.com>
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_<name>=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) <noreply@anthropic.com>
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/<key>` 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) <noreply@anthropic.com>
…he wild 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…nation 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…ry id 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
a2f2827 to
9f2b892
Compare
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Second half of a review comment I had only half-addressed: the opt-out behaviour shipped in 4f464dc, 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) <noreply@anthropic.com>
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 delta-verify at 54534d5
Diffed from R1 SHA a2f2827f through the 11 canary-scoped commits — all R1 findings addressed, plus James's out-of-directory concern and the telemetry-opt-out-as-canary-opt-out policy landed cleanly. CI now green across all lanes (Player perf, regression shards, Windows renders, CodeQL, CLI smokes, Producer/SDK tests).
| Finding | Resolution |
|---|---|
| Seed injection gated on telemetry, canary eval was not | feat(cli,studio): telemetry opt-out is canary opt-out — CLI resolves every canary to telemetry_opt_out when telemetry is off and publishes decisions to Studio via window.__HF_CLI_CANARY_DECISIONS. Divergence closed at the source. |
HF_CANARY_<FEATURE> env override didn't propagate to Studio |
Same commit — CLI applies env overrides during decision resolution, then injects decisions. Studio adopts them at canary.ts:80 (window.__HF_CLI_CANARY_DECISIONS?.[name]) with test coverage in canary.test.ts:233-265. |
studio:* events lacked canary properties |
feat(core): emit canary assignments as PostHog flag properties — moved from per-event opt-in to trackEvent's property-mixin at client.ts:77 (...canaryEventProperties()). Auto-tags every event with $feature/canary-<name>; native to PostHog flag surfaces. |
Unauthenticated /api/telemetry-identity exposed seed to any local caller |
isLoopbackHost() DNS-rebinding guard at telemetryIdentity.ts:69-78, applied at studioServer.ts:671. Handles [::1], dotted-quad, and the full 127.0.0.0/8 block. |
Partial corruption dropped bucketSeed even when salvageable |
config.ts:95-97 — corruption is now defined as BOTH markerAt AND bucketSeed absent; malformed markerAt alone synthesizes a fresh timestamp and keeps the seed. |
predecessorFound conflated "absent" and "corrupt" |
config.ts:280 — predecessorFound: state !== null || read === "corrupt"; corruption is now treated as "predecessor existed", with a new stateFileCorrupt field to distinguish. The docs commit 10536e847 notes override contamination as an innocent explanation for calibration check 3. |
writeConfig return ignored on backfill (unwritable dir loses seed) |
New writeConfigWithResult (config.ts:582) exposes ok/error; syncInstallState moved INSIDE writeConfigWithResult at line 591, so the state-file mirror is guaranteed to run only on successful atomic config write. Coupled at the source. |
autoUpdate.ts detached child bypassed writeConfig (breaker drop risk) |
The detached child now writes only config-only fields (completedUpdate, clears pendingUpdate) at autoUpdate.ts:105-118. Install-state (markerAt / bucketSeed / deParallelRouterTrialFired) lives in a separate file the child never touches — the race I was worried about no longer exists because the surfaces don't overlap. |
| FNV-1a UTF-16 vs UTF-8 discrepancy | canary.ts:85-92 — documented as ASCII-only-by-contract with an explicit widening path (TextEncoder in browser). Pragmatic given the browser bundle constraints. |
No test pinning de-parallel-router at 0% |
canary.test.ts:247-248 — it("keeps de-parallel-router at 0% until the circuit breaker is wired") with expect(findCanary("de-parallel-router")?.percentage).toBe(0). |
Also worth calling out:
- Out-of-directory state file (James's in-thread concern):
dae1b63d4+9f2b892a7move install-state insideCONFIG_DIR, sorm -rf ~/.hyperframesgenuinely resets everything. - Machine-lineage seed (
bucketSeed) is now decoupled fromanonymousId— commit8dd20ac2e. Correct primitive for cohort stability across a telemetry-id re-mint. - Calibration docs got extensive clarification (checks 2-4, override contamination, per-surface comparison, dashboard link, loose-toggle count in the registry header).
- The
canary-rollouts.mdxguide is still one of the clearer primitive docs in HF — the added trust-boundary discussion Vance surfaced in Slack should land in the runbook too.
LGTM from my side — clean, thoughtful rework of a subtle set of cross-surface concerns.
— Review by Rames D Jusso
miga-heygen
left a comment
There was a problem hiding this comment.
Re-review: PR #2854 — canary rollouts, post-feedback
Overview
+2518/−70 across core, CLI, Studio, and docs. Reviewed the full 3007-line diff with focus on the three concerns from James's feedback: state storage location, telemetry opt-out gate, and CLI→Studio canary agreement. All three are properly addressed.
The design is well-reasoned. The decision to publish decisions not inputs to Studio, the bucketing on feature:seed for independent slices, the telemetry-opt-out gate before bucketing, and the DNS-rebinding guard on the identity endpoint are all thoughtful choices.
1. State storage — ADDRESSED
The bucketSeed lives in install-state.json inside ~/.hyperframes/ alongside config.json. Deleting the config dir resets canary enrollment — test explicitly verifies: "the seed does NOT survive deleting the config dir — that reset is real."
The seed survives config.json re-mints (which happen on every command) — test verifies: "the seed survives a config.json re-mint — cohorts hold, only the id re-rolls."
The only ~/.local/state/ reference is a legacy migration path that adopts a pre-move seed and then deletes the legacy file. Clean.
2. Telemetry opt-out = canary opt-out — ADDRESSED
telemetryActive() checks BOTH the runtime override (HYPERFRAMES_NO_TELEMETRY, DO_NOT_TRACK, dev mode) AND the persisted preference (hyperframes telemetry disable). The check runs before bucketing — no cohort is ever assigned for opted-out installs. The CanaryDecision gets reason: "telemetry_opt_out" without a bucket field.
Studio mirrors this with its own isOptedOut() (localStorage flag).
An explicit HF_CANARY_* override still wins — checked first, before the telemetry gate. This is the documented path to exercise a canary with telemetry off.
3. CLI→Studio cross-surface — ADDRESSED
The CLI publishes window.__HF_CLI_CANARY_DECISIONS — a plain { name: boolean } map — and Studio takes it as authoritative. Three divergence cases eliminated:
- Telemetry off: CLI resolves
telemetry_opt_out, but Studio's opt-out is a separate localStorage flag. Without the injected decisions, Studio would evaluate independently. HF_CANARY_*override: Env vars never cross into the browser. Studio would not see them.- No seed injected: Studio falls back to a different unit id. Without the decisions, it would land in a different bucket.
Decisions are published even when telemetry is off — precisely the case they exist for. The identity block (distinct id, seed) is suppressed; the decisions block (booleans) is emitted. Test verifies this explicitly.
4. Rollout mechanism — CORRECT
FNV-1a bucketing: Canonical test vectors verified. Distribution validated via chi-square (30k UUIDs), share accuracy within 1pp, and binomial independence across 8 concurrent features. The feature:unitId hash input gives each canary an independent slice — tested that overlap at 10% × 10% ≈ 1%, not 10%.
Registry: Single file, one entry per rollout. Each carries name, percentage, description, owner, and sunsetAfter — the sunset date is enforced by a test (overdueCanaries()). Current entries: two calibration canaries (10%, 50%) and one real canary (de-parallel-router at 0%, guarded to stay at 0% until the circuit breaker is wired).
evaluateCanary() precedence:
- Override →
forced_on/forced_off(no bucketing) - Telemetry opt-out →
telemetry_opt_out(no bucketing, CLI/Studio only) pct ≤ 0→out_of_cohortexclude(CI/webdriver) →excluded- No unit id →
no_unit_id(fail-closed) pct ≥ 100→in_cohortbucket < pct→in_cohort/ else →out_of_cohort
Override-before-exclusion is correct: an explicit HF_CANARY_*=on reaches CI installs, which is how you test a canary in CI.
Ramping is inclusive: bucket < pct means widening 10→25 keeps everyone already at 10. Tested with set containment across ramp steps.
5. Telemetry integration — CORRECT
Every event carries /canary-<name>: "true" | "false" for ALL registered canaries. Both arms emitted ("false" vs absent distinguishes control from old-build). The canary- infix prevents namespace collision with real PostHog flags.
6. Security considerations — WELL-HANDLED
- DNS rebinding guard:
/api/telemetry-identitynow checksisLoopbackHost(c.req.header("host")). Comprehensive test coverage (localhost, 127.x.x.x, [::1], plus rejections for evil.example.com, localhost.evil.com, 192.168.x.x, 0.0.0.0). - Script injection:
JSON.stringifyfor the decisions map + test that a canary name containing</script>is escaped. - Seed not exposed over HTTP: The
/api/telemetry-identityendpoint deliberately does NOT servebucketSeed. Studio gets booleans, not the value cohorts derive from. - Resolver resilience: A throwing canary resolver does not break preview —
buildCliIdentityScriptcatches and continues without canary data.
7. Documentation — THOROUGH
The docs/contributing/canary-rollouts.mdx page (287 lines) is excellent — it covers adding a canary, overriding, measuring, cumulative exposure drift (with the math), the calibration experiment, removal, and the four pre-registered checks. The calibration checks are stated before the data arrives, which makes the read non-post-hoc.
Minor note
One test gap: Studio client.ts adds canary properties via ...canaryEventProperties() but has no dedicated client.test.ts test verifying they appear on emitted events (the CLI does). Low risk since the pattern is identical and the Studio canary tests validate canaryEventProperties() output directly.
Verdict
Approve. All three original review concerns are properly addressed — state inside the config dir, telemetry opt-out as canary opt-out (before bucketing), and CLI→Studio agreement via published decisions (not re-derivation). The rollout mechanism is correct (FNV-1a verified, independent slices, inclusive ramping, fail-closed). Security is well-handled (DNS rebinding, injection escaping, seed isolation). Test coverage is comprehensive across all layers. All required CI checks pass.
— Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed exact head 54534d53c257fda8fa0f14d4d91fd58724f6b6a6. The R2 direction is substantially better, but I found four blocking gaps:
-
P1 — DNS-rebinding guard is bypassed by the SPA response.
/api/telemetry-identityrejects a non-loopbackHost, but the catch-all SPA route still callsbuildStudioHeadScripts()and returns__HF_CLI_DISTINCT_IDplus the new__HF_CLI_BUCKET_SEEDin readable HTML for the same hostile Host. A rebound origin can fetch/instead of the guarded endpoint. Guard the SPA response or omit identity/seed injection for untrusted Hosts; add a route-level hostile-Host regression. -
P1 — a CLI decision can override Studio's own telemetry opt-out.
decideStudioCanary()accepts__HF_CLI_CANARY_DECISIONS[name]before checkingisOptedOut(). If CLI telemetry is enabled but Studio's documented localStorage opt-out is set, an injected percentage decision oftruestill enrolls Studio. A boolean map also loses the provenance needed to distinguish a deliberateHF_CANARY_*override from ordinary cohort enrollment. Preserve provenance or otherwise make Studio opt-out win for percentage enrollment while retaining explicit-override behavior, with the interaction matrix tested. -
P1 — the legacy
studio:*telemetry path is outside both new contracts.packages/studio/src/utils/studioTelemetry.tssends a large set ofstudio:*events directly. It neither addscanaryEventProperties()nor honors the documentedhyperframes-studio:telemetryDisabledkey (it checks only legacyhf-studio-telemetry-opt-out). This contradicts “every telemetry event carries the assignment” and means the documented opt-out still emits those events. Route this path through the shared client or share the same opt-out and canary-property SSOT. -
P2 — partial state salvage can drop a tripped breaker.
salvageInstallState()treats a record as corrupt when bothmarkerAtandbucketSeedare unusable, even ifdeParallelRouterTrialFired === true.mintConfig()then cannot restore the latch and can re-enroll a machine whose experimental router already failed. Treat the tripped bit as independently salvageable and add the damaged-marker/no-seed regression.
The exact-head CI is terminal green; these are behavioral/security gaps not covered by it. I also noticed the PR description and canary-rollouts.mdx:82-83 still describe superseded state/opt-out semantics, so please refresh those alongside the fixes. No merge or deployment performed.
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) <noreply@anthropic.com>
R3 blockers addressed —
|
| CLI decision | Studio opted out | Result |
|---|---|---|
| forced on/off | yes | honoured — documented escalation channel, same as a local URL override |
| cohort roll | yes | refused, telemetry_opt_out |
| cohort roll | no | adopted, in_cohort/out_of_cohort |
| none | yes | refused |
Full matrix tested. The precedence chain is documented on decideStudioCanary.
P1-3 — legacy studio:* path outside both contracts. Fixed at the source rather than by routing: utils/studioTelemetry.ts now calls isOptedOut() and keeps its legacy hf-studio-telemetry-opt-out check, so nobody already opted out gets quietly re-enabled, and it mixes in canaryEventProperties(). "Every telemetry event carries the assignment" is now actually true. Added studioTelemetry.test.ts (the module had no tests).
P2-4 — salvage could drop a tripped breaker. Right, and the worst of the four in consequence — losing the latch re-enrols a machine whose router already failed, which is the single thing install-state exists to prevent. All three fields are now independently salvageable; only a record where none survive is corrupt.
Docs. canary-rollouts.mdx:82-83 said "disabling telemetry disables the reporting, not the enrolment" — exactly backwards since the opt-out gate landed. Corrected, and I grepped for other copies of that claim (none). PR description refreshed below.
Verification: 257 CLI + 3,140 Studio + 24 core tests pass; typecheck, oxlint, oxfmt clean; fallow audit exit 0 (dead code 0, complexity 0 — decideStudioCanary hit 10 cyclomatic on the first pass and was restructured). Fault injection: reverting each fix independently fails 2 CLI / 1 Studio / 2 Studio / 2 CLI respectively.
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed exact head f81ab0162e2add6c278518de8dc237450b5f8d93.
The R3 delta correctly fixes three prior blockers: hostile-Host SPA responses no longer contain the CLI identity/seed, {enabled, forced} preserves override provenance so Studio opt-out can beat ordinary enrollment, and the tripped breaker is independently salvageable. I cannot stamp this head yet:
-
P1 — the required Test check is red.
packages/cli/src/server/studioServer.test.ts:90expects the hostile-Host SPA request to return 200, but the fixture never provides a Studio bundle, so the route returns the existing “Studio bundle missing” 500 page. CI reports 1 failed / 2320 passed / 2 skipped. Give this route test a realindex.htmlfixture (or mock the resolved bundle) so it actually exercises the injection branch, then rerun the required Test lane. -
P1 —
studio:*still bypasses several privacy opt-outs.packages/studio/src/utils/studioTelemetry.ts:40-47now honors both localStorage keys, but unliketelemetry/client.ts:60-68it still ignoresnavigator.doNotTrack,VITE_HYPERFRAMES_NO_TELEMETRY, Vite dev mode, and API-key eligibility. Those events can therefore send—andcanaryEventProperties()can evaluate—under controls the public docs say disable telemetry and enrollment. Extract a cycle-free browser telemetry policy shared by both transports, and add no-fetch/no-beacon/no-enrollment coverage for every opt-out. -
P1 — supported LAN/remote preview loses the authoritative CLI decisions.
HYPERFRAMES_PREVIEW_HOST=0.0.0.0is an explicit supported mode, butstudioServer.ts:824-827withholds the entire head script for every non-loopback Host. That drops the safe{enabled, forced}map along with identity/seed, so Studio re-derives locally and can disagree with CLI opt-out, cohort, or env overrides again. Split non-identifying decision injection from identity/seed injection (or reject non-loopback Studio access entirely), and cover the supported remote-host matrix. -
P1 — the install-state breaker is not authoritative or truthfully persisted.
writeConfigWithResult()returns success even whensyncInstallState()fails, and the safety retry inrender.ts:1254-1262verifies onlyconfig.json. Conversely, normal reads takedeParallelRouterTrialFiredonly fromconfig.json; they do not rehydrate a latchedtruefrom install-state. A failed mirror—or a stale concurrent config writer after the breaker fires—can therefore leave state latched but config false, and a later process re-enrolls. Make the safety write verify both stores and/or merge the latched state into every effective read; add deterministic state-write-failure and stale-writer regressions. -
P2 — the public contracts still describe superseded behavior.
canary-rollouts.mdx:119-121says a config wipe loses the breaker, while:184-197says the opposite;:225-233documents a{name:boolean}map and unconditional CLI precedence instead of{enabled,forced}plus the current precedence. The PR body still names~/.local/state/..., says state survives deleting~/.hyperframes, describes the old identity/seed bridge, and has stale validation counts. Please update both before landing; also state explicitly that deliberateHF_CANARY_*/ URL overrides are the exception to telemetry opt-out.
Security note, not charged to this PR: explicitly LAN-bound Studio APIs remain broadly unauthenticated. The new Host check is a DNS-rebinding mitigation for identity-bearing responses, not general access control.
No merge or deployment performed.
P1 — the required Test lane was red, and it was my test. The
hostile-Host SPA case asserted a 200, which only holds when
packages/studio/dist is built: true on a dev box, false in CI, so it
passed locally and failed there. The Host split moved into a pure
buildStudioHeadScriptsForHost() and is asserted directly; the route test
no longer depends on build state. Verified by running the CLI suite with
the bundle moved aside — 2330 pass.
P1 — studio:* still bypassed most privacy controls. It honoured two
localStorage keys but not navigator.doNotTrack,
VITE_HYPERFRAMES_NO_TELEMETRY, Vite dev mode or API-key eligibility, and
canary enrolment honoured a different single control. New
telemetry/policy.ts is the one answer to "may this profile be measured",
consumed by both transports and by enrolment. It imports only ./config,
so no cycle with the modules that import it. Each control is asserted
individually.
P1 — LAN/remote preview lost the authoritative decisions. Withholding
the whole head script for any non-loopback Host also dropped the safe
{enabled, forced} map, sending a supported HYPERFRAMES_PREVIEW_HOST=
0.0.0.0 Studio back to re-deriving. Identity injection is now gated
separately from decision injection: identity is loopback-only, decisions
always publish.
P1 — the breaker latch was neither authoritative nor truthfully
persisted. syncInstallState swallowed its own failures so
writeConfigWithResult always reported ok, and reads took the flag only
from config.json. The latch is now merged into every effective read,
which makes install-state authoritative and closes both the failed-mirror
and stale-concurrent-writer paths; the write additionally reports
mirrored: false rather than swallowing.
P2 — public contracts. canary-rollouts.mdx said a config wipe loses the
breaker (it does not) and documented the superseded {name: boolean} map
with unconditional CLI precedence; both corrected, with the precedence
ladder written out and the override exception stated explicitly. PR body
rewritten — it still named ~/.local/state, claimed state survives
deleting ~/.hyperframes, and carried stale counts.
Tests: 2330 CLI (bundle absent), 3151 Studio, 24 core. Fault injection:
reverting each fix alone fails 5 CLI / 5 Studio.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
R4 blockers addressed —
|
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 5e2a9432f142416de7d3d72f0d6b08ad999a23e4.
The R4 delta fixes the bundle-dependent Test failure, routes both Studio transports and canary enrolment through the shared browser policy, preserves non-identifying CLI decisions for LAN/remote Hosts, and refreshes the public contracts. One safety blocker remains in two connected pieces:
-
P1 — a negative install-state latch is cached forever in a long-lived process.
config.ts:150-170memoizes bothtrueandfalse, but onlytrueis monotonic across processes. Sequence: process A reads unlatched state and cachesfalse; process B trips and mirrors the breaker; a stale writer clearsconfig.json; A callsreadConfigFresh(), which clears onlycachedConfig, reuses the cachedfalse, and can re-enrol in the experiment that already failed. Cache onlytrue, or invalidate/re-read the negative latch on a fresh effective read. Add the same-modulefalse → external state true → stale config false → fresh read trueregression. -
P1 — the real breaker writer still discards
mirrored:false.writeConfigWithResult()exposes a failed install-state mirror, butrender.ts:1254-1262,1313-1319callswriteConfig(), which collapses{ok:true, mirrored:false}to success. The follow-up fresh read seesconfig.json:trueand returns before retrying or warning. A later stale write/corruption can then erase the only durable copy. ConsumewriteConfigWithResult()in this safety path, require/retry the mirror, and cover mirror-failure followed by stale-clobber through the real caller.
The added tests separately preload an already-true state and assert that mirrored:false is returned, so neither transition is currently exercised. Targeted config/policy tests pass and required Build/Test/Lint/Typecheck/CodeQL are green; several Windows/regression jobs are still running. No merge or deployment performed.
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) <noreply@anthropic.com>
R5 blockers + independent review findings —
|
What
A shared, percentage-based rollout mechanism: enable a change for a stable slice of installs instead of all-or-nothing, decided locally and offline (the render path forbids network fetches), reported to PostHog as native flag properties (
$feature/canary-<name>) so breakdowns/funnels/experiments work with nothing configured server-side.packages/core/src/canary.ts— pure evaluator (FNV-1a overfeature:installId,bucket < pct). No fs/network/process; browser-safe.packages/core/src/canaryRegistry.ts— every rollout in one table: name, percentage, description, owner,sunsetAfter(past-due = failing test).packages/cli/src/telemetry/canary.ts— CLI binding:anonymousId,HF_CANARY_<FEATURE>override, CI exclusion.packages/studio/src/telemetry/canary.ts— browser binding: Studio distinct id (adopts the CLI id when CLI-launched, so both surfaces share a cohort),?hf_canary_<name>=onoverride (session-scoped),navigator.webdriverexclusion."false"= control, absent = build predates the canary).@hyperframes/core/canary{,-registry}viapackage-subpaths.json— the core barrel breaks the studio browser bundle.Properties (all tested): inclusive ramping (widening never drops an enrolled install), independent slices per feature, fails closed (no id / unknown name / CI → off), memoized per process/page load.
Calibration experiment
Two inert canaries (
calibration-10@10%,calibration-50@50%) gate nothing and validate the mechanism on real traffic before anything depends on it. Four pre-registered checks (accuracy, cumulative-exposure drift, cohort stability, cross-surface agreement) + an independence check — documented indocs/contributing/canary-rollouts.mdx, with matching dashboard tiles already built.de-parallel-routeris registered at 0% but deliberately unwired — wiring it through the CLI trial is follow-up work after calibration reads clean.Install-state (breaker + cohorts)
config.jsonis hot and wide — ~20 fields rewritten on every command and every render — andreadConfigrecovers from any parse/permission/IO failure by minting a fresh identity. Two facts must not ride on that, so they live in~/.hyperframes/install-state.json: a separate file, deliberately the same directory, sorm -rf ~/.hyperframesis a genuine full reset. (An earlier revision put it under~/.local/state/to survive that delete; review rejected persisting state to defeat the user's own reset, and the data agreed —install_predecessor_foundreadsfalseon 155k/155k events, so the wipe carryover has never once fired.)deParallelRouterTrialFired— a tripped breaker stays tripped across aconfig.jsonre-mint. Merged back in on every effective read, which makes install-state authoritative for the latch rather than a mirror: a failed write, or a stale concurrent writer rewriting config from a pre-trip snapshot, can no longer re-enrol a machine whose router already failed.bucketSeed— the canary bucketing unit, a dedicated UUID distinct fromanonymousId, write-once. Canaries hashfeature:seed, so cohorts hold across a re-mint. Never emitted.markerAt— written unconditionally, so the fraction of fresh mints finding it measures re-mint churn directly. Emitted asinstall_predecessor_found, withinstall_state_file_corruptsplitting "we lost this machine's record" from "genuinely new machine".All three are independently salvageable from a partially corrupt record — most importantly the tripped bit, since dropping it is the one outcome the file exists to prevent. The file holds no telemetry identity. Migration from the pre-move location adopts and deletes it.
CLI → Studio
The CLI publishes
window.__HF_CLI_CANARY_DECISIONS—{ name: { enabled, forced } }— and a launched Studio adopts it rather than re-deriving, which could not agree with the CLI when telemetry is off, when anHF_CANARY_*override is set, or when no seed was injected.forcedcarries provenance so a deliberate override outranks Studio's own opt-out while an ordinary cohort roll does not.Identity is treated separately from decisions:
__HF_CLI_DISTINCT_ID/__HF_CLI_BUCKET_SEEDgo out only for a loopbackHost(DNS-rebinding mitigation, applied to the SPA route and the/api/telemetry-identityendpoint alike — which no longer serves the seed at all). The decisions map is non-identifying and always published, so a LAN preview (HYPERFRAMES_PREVIEW_HOST=0.0.0.0) stays in agreement with the CLI.Telemetry opt-out is canary opt-out
Enrolment is part of measurement: an install that reports nothing cannot be compared, so bucketing it changes that user's code path for no signal. One shared browser policy (
packages/studio/src/telemetry/policy.ts) now governs both Studio transports and enrolment — documented and legacy localStorage keys,navigator.doNotTrack,VITE_HYPERFRAMES_NO_TELEMETRY, Vite dev mode, API-key eligibility — with the CLI mirroringshouldTrack()'s inputs. Resolves totelemetry_opt_outbefore bucketing, so no cohort is assigned at all.The documented exception: an explicit
HF_CANARY_*(or?hf_canary_*=in Studio) override still wins. It is a deliberate operator choice — the escalation channel for dogfooding, bisects and panic-off — not silent enrolment.Validation
Canary + carryover + policy tests across core, CLI and Studio: canonical FNV-1a vectors plus a cross-check that the shipped bucket function uses that hash, ±0.16pp accuracy at n=60k, chi-square uniformity 89.0 vs 148.2 critical, binomial independence across 8 concurrent canaries, real fleet ids landing 9.9/24.8/49.5 against 10/25/50 targets. Behavioural coverage: latch rehydration and the stale-writer race, partial-corruption salvage per field, backfill write failure, the full opt-out precedence matrix on both surfaces, each privacy control individually, and the Host split for identity vs decisions. Every fix in the review rounds has a regression that fails when that fix alone is reverted.
🤖 Generated with Claude Code