fix(cli): refresh identity persistence classification - #3069
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Review: identity persistence reclassification
Clean corrective follow-up to #3065. The original design locked the classification to the process lifetime; this PR keys it to the anonymous ID, so a long-running process that replaces its ID (config deletion, storage recovery) correctly reclassifies the replacement.
The invariant change, and why it's safe
Before (process-level stickiness):
if (identityPersistence !== undefined) return;
Once classified, never changes. This is too aggressive — if a readConfigFresh() mints a new ID (e.g. config deleted between calls), the NEW id inherits the OLD id's classification.
After (per-ID stickiness):
if (identityClassification?.anonymousId === anonymousId) return;
Same ID → guard holds (anti-self-promotion intact). Different ID → reclassification proceeds.
Traced all four classification call sites through readConfig():
| Path | ID from disk? | Classification | Reclassifies on new ID? |
|---|---|---|---|
No config file → mintAndCacheConfig() |
N/A | unknown or process_only |
✅ |
| Existing file, needs seed backfill, id on disk | yes | durable |
✅ |
| Existing file, needs seed backfill, id minted | no | by write outcome | ✅ |
| Existing file, no backfill needed | either | durable or process_only |
✅ |
| Existing file corrupt → catch recovery | N/A | by write outcome | ✅ |
Every site passes config.anonymousId as the key. The guard correctly skips when the ID matches (stickiness) and proceeds when it differs (reclassification).
Anti-self-promotion still works
The critical scenario from #3065's review:
- No config → mint ID "A" → classify as
unknown readConfigFresh()→ cache cleared → file exists (we wrote it) → reads ID "A" →classifyIdentity("A", "durable")→ guard:"A" === "A"→ rejected → staysunknown✅
The original test (line 751) still covers this. The new expect(getIdentityWriteOutcome()).toBe("ok") assertion at line 38 tightens the existing test.
New test coverage
"reclassifies a new id after a durable install's config is deleted" (line 41):
- Loads existing config → durable
- Deletes config file, calls
readConfigFresh() - New ID minted → correctly classified as
unknownwithokwrite outcome - Asserts
replacement.anonymousId !== first.anonymousId
"reclassifies a new id after process-only storage recovers" (line 57):
- Writes fail → process_only/failed
- Writes recover, calls
readConfigFresh() - New ID minted → correctly classified as
unknownwithokwrite outcome - Asserts
replacement.anonymousId !== first.anonymousId
Both tests verify the scenario that #3065's process-level stickiness got wrong. Good regression coverage.
Getter safety
The getters add a second ID-match check:
return identityClassification?.anonymousId === config.anonymousId
? identityClassification.persistence
: "unknown";This is belt-and-suspenders — if the config's ID somehow diverges from the classification's ID (shouldn't happen in normal flow, but could via direct writeConfig mutation), the getter returns the safe default rather than a stale classification. Correct defensive design.
process_only description update
The description expanded from "the write failed" to "not persisted by the identity-establishing path (the write failed or no write occurred)." This correctly covers the no-backfill-needed path at line 748 where a minted replacement ID has no write opportunity — it's genuinely process-only.
CI note
Two infra-level timeouts (Detect changes 52m, Analyze javascript-typescript 52m) — runner resource issues, not code-related. Core checks passing: CLI smoke, Producer tests (unit + integration), Typecheck, Lint, Format, Windows tests/render. A few still pending (Build, Test, SDK).
No blocking concerns. Clean correction that preserves the stickiness invariant while fixing the reclassification gap.
— Miga
vanceingalls
left a comment
There was a problem hiding this comment.
R1 review — LGTM, corrective follow-up cleanly narrows classifier keying and covers both replacement paths
Head reviewed: ecf8d62
Requester claims — verdict per claim:
- Classification-keying to active anonymous ID: CORRECT
- Reclassifies replacement ID after config deletion or storage recovery: CORRECT
- Same-ID stickiness intact: CORRECT
- No change to event names / distinct_id / render events / dashboard semantics: CORRECT
- Follow-up to #3065: CORRECT
Lens 1 — classification-keying (CORRECT)
packages/cli/src/telemetry/config.ts:579-586 — classifyIdentity(anonymousId, persistence, writeOutcome) early-returns iff identityClassification?.anonymousId === anonymousId. Different id → overwrites the whole record. The tuple {anonymousId, persistence, writeOutcome} moves as one unit, so a stale outcome can never survive an id transition.
All four writer call-sites updated to pass config.anonymousId: packages/cli/src/telemetry/config.ts:354-358 (mint-and-cache), 732, 734-738, 748, 760-764 (corrupt-recovery). No orphaned two-arg call left in the tree.
Lens 2 — same-ID stickiness (CORRECT)
Trace on the ephemeral-HOME fresh-mint case (the primary case that motivated unknown in #3065):
mintAndCacheConfigclassifies X asunknown/ok.cachedConfig = {X}.readConfigFresh()clears the cache;readConfigre-parses the just-written file;idFromDisk=true,bucketSeedpresent → hits thepackages/cli/src/telemetry/config.ts:748no-write branch →classifyIdentity(X, "durable").- Early-return fires (previous anonymousId === X). Classification stays
unknown.
The new assertion at packages/cli/src/telemetry/config.test.ts:757 (expect(getIdentityWriteOutcome()).toBe("ok") added to the existing sticky test) pins the outcome half of the tuple too — not just the persistence label — closing the gap where a hypothetical bug could preserve persistence and drop writeOutcome.
Lens 3 — no dashboard-semantics change (CORRECT)
packages/cli/src/telemetry/client.ts:113,116-117 — the only client-side changes are comment refinements on identity_persistence and config_write_outcome. No field rename, no new field, no call-site change on posthog.capture, no distinct_id touch, no render-event delta. Grepped the diff for capture|identify|distinct_id|render_ — zero hits outside comments. The three-valued domain of identity_persistence (durable | process_only | unknown) is unchanged; dashboards computing rates over that domain will see the SAME set of possible values. The data distribution shifts on churn workloads (correctly — replacement ids after deletion now report unknown instead of stale durable), which is the fix's intended effect.
Lens 4 — adversarial internal-boundary audit at the fix's execution graph
Applied feedback_adversarial_audit_fix_internal_boundaries — walked every remaining silent-misclassification path inside the fix's own boundary:
- Reader-writer symmetry (
getIdentityPersistence/getIdentityWriteOutcomeatconfig.ts:594-611): both callreadConfig()FIRST, then checkidentityClassification?.anonymousId === config.anonymousId. IfreadConfig()returned the cached path (no re-classify), classification already matches the cached id. If it re-entered the parse/mint path, classification is set/overwritten to the current id in that same call. The check is defensive — an actual observable divergence would require a mutation ofcachedConfigoutsidereadConfig/writeConfigWithResult, which grep confirms doesn't happen (cachedConfig =only atconfig.ts:359,742,750,778,818). - N > 2 replacements in one process: Each transition overwrites
identityClassificationwholesale; there is no per-id memory to leak. All later reads see only the latest tuple. Fine. - Corrupt-config recovery branch (
config.ts:752-766): now passesconfig.anonymousIdintoclassifyIdentity, so the recovery's minted id gets its own classification. Pre-fix bug (stale first-verdict wins) is closed here too. - Concurrency: All FS ops are sync; Node single-threaded; no race window inside the classify/read cycle.
- First-run vs replacement: first-run mints X,
classifyIdentity(X, "unknown", "ok")— distinguishable from any later reclassify becauseidentityClassificationstartsundefined.install_predecessor_found=falsefrommintConfig()(unchanged) distinguishes genuine-first vs recovered-machine cases at the event layer. - Backward compat: old rolling processes on the pre-#3069 build continue to emit their sticky-per-process verdict. That's the pre-fix behavior for those processes — dashboards already accept it. New builds emit the corrected verdict. No mixed-population contract break.
Lens 5 — PR envelope
- Single commit by
james.russo@heygen.com(jrusso1020). NoCo-Authored-By:trailers, noGenerated with [Claude Code]footer. Envelope clean. - PR body uses
## Summary+## Verification— same shape as parent PR #3065 which merged fine; hyperframes' What/Why/How/Test-plan template is not enforced by CI. Not blocking. - CI at review time:
Build,Render on windows-latest,Semantic PR title,Tests on windows-latest,Typecheckall SUCCESS.Test,Test: runtime contract,regressionstill open. Approve here is code-review-only; merge should await the 3 open required gates.
Lens 6 — Standards checklist (mechanical)
Grepped the diff for \bas +[A-Z], \w+!\., \w+!\[, angle-bracket casts, and .message on untyped catch. Results:
- Bare
as Ton changed lines: 0 residual (the existingJSON.parse(raw) as Partial<HyperframesConfig>atconfig.ts:711is unchanged and outside the fix's touched lines). - Non-null
!assertions on changed lines: 0. - Angle-bracket casts: 0.
catch (e) { e.message }without narrowing: 0. Recovery branch uses barecatch { ... }— no error-value access.
Test coverage — does each new test fail at pre-patch HEAD?
packages/cli/src/telemetry/config.test.ts:762-777(durable → deleted config → new id): pre-patchclassifyIdentityearly-returned onidentityPersistence !== undefined, so the second classification would be ignored and the assertionexpect(getIdentityPersistence()).toBe("unknown")would see stale"durable". FAILS pre-patch, passes post-patch.packages/cli/src/telemetry/config.test.ts:779-799(process_only → storage recovers → new id): symmetric — pre-patch would return stuck"process_only". FAILS pre-patch, passes post-patch.
Both tests genuinely exercise the bug being fixed, not just tautologies.
Findings summary: 0 blockers, 0 non-blocker fixes. All five requester claims verified as CORRECT.
Review by Via
ecf8d62 to
7640adc
Compare
Summary
This is a corrective follow-up to #3065. It does not change event names, distinct_id, render events, PostHog queries, or dashboard semantics.
Verification