feat(cli): classify identity persistence on every telemetry event - #3065
feat(cli): classify identity persistence on every telemetry event#3065WaterrrForever wants to merge 1 commit into
Conversation
Install-grain metrics currently trust every anonymousId equally, but ephemeral/isolated-HOME workloads mint a fresh id per run — one machine produced 2,956 rotating render identities since Jul 30 (94.4% seen on a single render command), inflating acquisition and diluting per-install penetration while looking like real product usage. Every event now carries: - identity_persistence: durable (id loaded from a preexisting config — proven to survive a process boundary) | unknown (minted+persisted this run; an ephemeral HOME is indistinguishable from a genuine first run from inside one process) | process_only (persist failed). Sticky per process so a fresh install re-reading its own write cannot self-promote. - config_write_outcome: ok | ok_unmirrored | failed for the identity- establishing write; absent when the id came from disk. - invocation_id: random uuid per CLI process, so one invocation's events group even when the install identity is untrustworthy (unlike run_id, which needs an orchestrator to set HYPERFRAMES_RUN_ID). Install metrics can then count only durable identities, and a daily churn monitor can alert on the unknown share.
miga-heygen
left a comment
There was a problem hiding this comment.
Review: identity-persistence telemetry
This is a faithful implementation of the telemetry spec James laid out in the thread — three-way identity classification, config write outcome, always-on invocation id. Traced every code path; the design is clean.
What I verified
Stickiness invariant — the anti-self-promotion guard. The core insight: classifyIdentity() uses a first-write-wins guard (if (identityPersistence !== undefined) return). Traced all three call sites through readConfig():
| Path | Classification | Correct? |
|---|---|---|
No config file → mintAndCacheConfig() |
unknown or process_only (by write outcome) |
✅ |
Existing file parses → materializeConfig |
durable |
✅ |
| Existing file corrupt → catch recovery mint | unknown or process_only (by write outcome) |
✅ |
The self-promotion attack vector — readConfigFresh() clearing the cache, re-entering readConfig(), finding the just-written file, hitting the existing-file path — is blocked by the guard. Test on line 751 covers this explicitly. Solid.
writeOutcomeOf mapping. ConfigWriteResult → IdentityWriteOutcome:
ok: false→"failed"✅ok: true, mirrored: undefined→"ok"✅ (undefined === falseisfalse)ok: true, mirrored: false→"ok_unmirrored"✅
getIdentityPersistence() forces classification. Calls readConfig() before returning, so it's impossible to read the property before classification has run. The ?? "unknown" fallback is unreachable in practice (every readConfig() exit calls classifyIdentity), but correct as a defensive default.
invocationId is simple and correct — ??= lazy init with randomUUID(), no state to leak across processes.
Test coverage. 5 tests cover all three persistence classes, the self-promotion guard, and corrupt-config recovery. Each test uses vi.resetModules() to get fresh module-level state — proper isolation for sticky-per-process semantics. The ok_unmirrored write outcome isn't directly tested (requires mocking partial mirror failure), but the writeOutcomeOf logic is trivial enough that the other paths cover it.
Mock additions. client.test.ts and client.postureRefresh.test.ts both add getIdentityPersistence: () => "durable" and getIdentityWriteOutcome: () => undefined to their config mocks — sensible defaults that don't distort existing test behavior ("durable" + no write outcome = the happy-path classification for an established install).
SSOT check
- Classification logic lives in exactly one place (
classifyIdentity). Three call sites each provide the correct value for their path — no duplicated decisions. writeOutcomeOfis the single translator from write-result shape to telemetry value.identity_persistenceandconfig_write_outcomeare emitted from one location intrackEvent— no scattered property injection.
Observations (non-blocking)
-
Corrupt-config catch path doesn't cache. The catch block at line 720-724 returns without setting
cachedConfig, meaning a subsequentreadConfig()call re-reads and re-parses the (now-recovered) file. The stickiness guard prevents re-classification, so this is safe — but it's a minor perf asymmetry vs. the other paths that all cache. -
readConfig()called 4× per event emission.trackEventhitsreadConfig()viashouldTrack(),readConfig().predecessorFound,readConfig().stateFileCorrupt, and thengetIdentityPersistence()/getIdentityWriteOutcome()each call it again. All cache hits after the first, so negligible cost — just noting the pattern.
CI is green across the board. No blocking concerns. Clean implementation of the spec.
— Miga
jrusso1020
left a comment
There was a problem hiding this comment.
Requesting changes on one finding. Terminal-green at c244d647e: all 8 required contexts pass (Build, Test, Test: runtime contract, Typecheck, regression, Semantic PR title, Render on windows-latest, Tests on windows-latest), enumerated from the branch ruleset with cancelled runs filtered out. The blocker is in the classifier, not CI.
Miga covered the stickiness invariant and the SSOT shape, so I will skip those. One claim from that review I have to contradict, though: the three readConfig() exit paths do not all classify correctly.
Blocking: the durable branch can classify an id that was minted this run
materializeConfig mints a replacement id whenever the parsed config does not carry a usable one:
// config.ts:674
anonymousId: parsed.anonymousId || randomUUID(),and readConfig's existing-file branch classifies unconditionally right after it:
// config.ts:693, then :698
const config = materializeConfig(parsed);
...
classifyIdentity("durable");So a config.json that exists and parses but carries no usable anonymousId reports identity_persistence: "durable" for an id that was minted this process and has survived nothing.
It is worse than a one-run mislabel, because that id usually never reaches disk. The only write on this branch is the bucket-seed backfill at :703-710; when bucketSeed is already present, from config.json or from install-state via guardedFields at :666, the branch falls through to cachedConfig = config at :712 with no write at all. That install then mints a fresh id on every run, persists none of them, and reports durable every time. It is the exact churn signature this PR exists to expose, wearing the one label that is supposed to mean "already survived a process boundary."
On reachability, being straight about it: the CLI cannot produce this config itself, since anonymousId is required on HyperframesConfig and writeConfigWithResult stringifies the whole object. So the population is hand-edited, provisioned, or image-baked configs, plus anonymousId: "". I cannot size it from here. What makes me want it closed before merge rather than after is that it fails silently in the one direction the field is meant to be trustworthy in, and the PR's own stated next step is switching install-grain tiles to durable-only counting. Once that lands, a mislabeled install is indistinguishable from a real one in the tiles, permanently. Nothing downstream would ever surface it.
The fix is local to that one line, using a helper already in the file:
classifyIdentity(parseNonEmptyString(parsed.anonymousId) !== undefined ? "durable" : "unknown");Worth a sixth test next to the five you added: preexisting config carrying a bucketSeed and no anonymousId, expect unknown. That is precisely the case the current "loaded from a preexisting config is durable" test cannot catch, since it supplies anonymousId: "prior-id".
Non-blocking
The corrupt-recovery swap is behavior-identical, in case it reads as a change. writeConfig(config) becoming writeConfigWithResult(config) at :721 looks like it could have dropped a warning, but writeConfig is just writeConfigWithResult(config).ok (:756-757) and neither one warns. The mint path's warnSeedBackfillFailed was always the only warning here. Nothing lost.
The contract audit on the two new config.js exports comes out clean. Four telemetry tests mock ./config.js: client.test.ts, client.postureRefresh.test.ts, canary.test.ts, events.test.ts. Only the first two run the real trackEvent (events.test.ts mocks ./client.js outright, canary.test.ts never calls it), and those two are exactly the ones this PR updates. Nothing half-landed.
getIdentityPersistence()'s ?? "unknown" fallback. writeConfigWithResult populates cachedConfig at :776, so a process that writes config before its first readConfig() leaves the verdict unclassified and the getter answers unknown for what may well be a durable install. That is the safe direction and I would leave it as is, but noting it so nobody later "fixes" it by making the fallback durable.
The taxonomy itself is the right shape: three-way with unknown as the honest middle, sticky per process, never promotable from inside a single process. Happy to re-review as soon as the durable branch checks that the id actually came off disk.
— Rames Jusso
| // The id was loaded from a file that predates this process — the one | ||
| // case where cross-run persistence is already proven. Sticky, so a | ||
| // fresh-install process re-reading its own write cannot self-promote. | ||
| classifyIdentity("durable"); |
There was a problem hiding this comment.
Blocking. This classifies durable unconditionally, but materializeConfig two lines up mints a replacement id when parsed.anonymousId is missing or empty (:674), and on this branch that id is only written back if bucketSeed also happens to be missing (:703-710). With a seed present the branch falls through to cachedConfig = config at :712 with no write, so such an install mints a fresh id every run, persists none, and reports durable every time.
classifyIdentity(parseNonEmptyString(parsed.anonymousId) !== undefined ? "durable" : "unknown");See the review summary for reachability and the suggested sixth test.
Summary
Implements the CLI half of the identity-churn remediation discussed on the leadership dashboard: since Jul 30, ephemeral/isolated-HOME workloads have been minting a fresh anonymousId on every run — one machine produced 2,956 rotating render identities (94.4% seen on exactly one render command), inflating acquisition and diluting per-install skills penetration while its render events look like real product usage. Similar high-churn candidates exist in at least three other locations, so a fingerprint denylist is remediation, not policy. The durable fix is to let PostHog tell trustworthy identities apart, per the review guidance on PR-adjacent threads: emit
identity_persistence = durable | process_only | unknown, the config/state write outcome, and a per-run id.What every event now carries
identity_persistence— can this process's anonymousId be trusted to survive to the next run?durable: the id was loaded from a preexisting config file — it has already survived a process boundary.unknown: minted this run and the write landed. An ephemeral HOME looks identical to a genuine first run from inside one process, so this is deliberately never promoted to durable.process_only: minted this run and the write failed (read-only mount, full disk) — the id dies with the process.config_write_outcome—ok | ok_unmirrored | failedfor the identity-establishing write (ok_unmirrored: config.json landed, install-state mirror didn't). Absent when the id came from disk.invocation_id— random uuid per CLI process. Groups one invocation's events even when the install identity is untrustworthy; unlikerun_idit needs noHYPERFRAMES_RUN_IDplumbing.How the churn workloads land in this taxonomy
Every run of an ephemeral-HOME workload is a fresh mint →
unknown(orprocess_onlywhen the FS is read-only), neverdurable. A legit new user isunknownon run 1 anddurablefrom run 2 on. Install-grain metrics can then countdurableidentities only, and a daily churn monitor can alert on theunknownshare.Test plan
config.test.ts: fresh-mint→unknown/ok, preexisting→durable/no-outcome, failed-write→process_only/failed, no self-promotion after readConfigFresh re-reads the process's own write, corrupt-config recovery classified by write outcome.packages/clisuite: 2,468 passed; build, oxlint, oxfmt clean.Follow-ups (not in this PR)
unknownshare.transcribekeeps a stable id whilerenderrotates) suggests the workload isolates HOME for render invocations specifically;invocation_id+config_write_outcomeshould make that mechanism visible in the data.