feat(auth): identify accounts and add structured login telemetry#212
Conversation
…nd structured login events Switch the analytics client's distinct id to the same cross-tool install identity already shared by the other HeyGen CLIs (promoting any existing local id on first run so upgraders keep their identity), and point events at the same ingestion project so a person's activity is queryable in one place instead of two. Add a `surface` property to distinguish this CLI's traffic there, plus IdentifyAccount/AuthLoginStarted/AuthLoginCompleted/ AuthLoginFailed methods so a login attempt becomes structured, queryable telemetry (method + specific failure reason) instead of a generic event.
Link a successful login (OAuth or API-key) to the account identity via PostHog identify, disclose telemetry + identity linking on first run, and emit structured started/completed/failed events per login attempt with method and failure reason so login funnels are queryable by branch instead of a generic command-complete event.
…vent runAPIKeyLogin and runOAuthLogin had several early-return error branches (malformed credentials file, save/clear failures, PKCE/state generation, loopback startup, authorize-URL build, response encoding) that returned without emitting AuthLoginCompleted or AuthLoginFailed, leaving their AuthLoginStarted event dangling with no outcome. Rather than instrumenting each scattered return site, wrap both runners with a deferred catch-all: a per-call `reported` flag is set at each existing AuthLoginCompleted/AuthLoginFailed call, and a defer fires AuthLoginFailed(method, "internal_error") only when the function returns an error and nothing else already reported an outcome. New error paths added later are covered automatically instead of silently reintroducing the gap.
miguel-heygen
force-pushed
the
feat/conversion-telemetry
branch
from
1d0a531 to
857dbd3
Compare
runOAuthLogin and runAPIKeyLogin marked the login as completed and fired AuthLoginCompleted before writing the success response via ctx.formatter.Data. If that write failed (e.g. a broken pipe on stdout), the command still exited non-zero but telemetry had already recorded a completed login. Reorder so completion is only reported once the response write actually succeeds; a write failure now falls through to the existing deferred reconciliation, which reports AuthLoginFailed instead.
There was a problem hiding this comment.
Reviewed the full diff (all 8 files), traced the auth + telemetry flow end-to-end, and cross-checked against the hyperframes scheme + posthog-go v1.11.2. Code LGTM — no correctness blockers. Fire-and-forget contract holds, login behavior unchanged, and no secret/token/PII in event properties (AUTH_LOGIN_* carry only method + a fixed reason enum + base props). The email-as-distinct_id is the intended, disclosed cross-tool identity.
✅ Update (re-review 2026-07-10): email-case fix landed and is symmetric.
identityKeynowstrings.ToLower(...)on both email and username (test:Demo@Example.com), and the hyperframes side lowercases too — so the cross-tool PostHog join holds without case-split profiles. This was the one substantive follow-up; it's resolved.On the red
govulncheckcheck — not introduced by this PR. #212 touches none ofupdate.go,go.mod,go.sum, or the Go toolchain. Both findings trace entirely through untouched code:
GO-2026-5932—golang.org/x/crypto/openpgp(unmaintained) via the self-update path (cmd/heygen/update.go). Fixed-in: N/A → needs migration off openpgp or an accepted-risk suppression, as a separate item.GO-2026-5856—crypto/tlsECH advisory in the stdlib, fixed in go1.25.12 (CI runs 1.25.11) → toolchain bump clears it.These are recent 2026 advisories the live vuln DB just began flagging repo-wide; they'd fail on any branch right now. Recommend a separate housekeeping ticket rather than scope-creeping this telemetry PR — and confirm whether govulncheck is a required merge gate.
Non-blocking nits:
internal/analytics/analytics.go—IdentifyAccountmutatesc.identifiedwithout synchronization. No real race today (single-goroutine login); a one-line comment on the assumption would harden intent.main.go:98-103— per-tool telemetry notice: a user who saw it via media-use sees it again from heygen-cli. Product decision, flagging so it's conscious.auth_login.godevice-code path fires STARTED + FAILED for the unsupported flag each invocation — fine, just confirm you want device-code attempts in the funnel.
— Somu
…join An uppercase-cased email or username would otherwise split one HeyGen account across two PostHog profiles, since hyperframes-oss's side of the identity join already lowercases.
os.UserHomeDir reads USERPROFILE on Windows, not HOME, so these tests leaked the real CI runner's home directory on windows-latest.
|
E2E verification (complement to Somu/tai's code passes; deferring approval to Terence/Somansh per the cross-tool-telemetry policy). Ran
Merge-ready on the e2e dimension. — Rames Jusso |
|
Live person-merge — now confirmed (closes the "didn't push to prod PostHog" caveat in my earlier e2e comment). Authenticated the test account via real OAuth (PKCE loopback, headless Chrome) on CLI Both ids resolve to one person Only remaining unproven-live detail: — Rames Jusso |
Switching heygen-cli's write-key to hyperframes-oss's shared project would silently flatline heygen-cli's own existing dashboard the moment a client upgrades, since no dashboard/query migration has happened yet. Fan every event out to both destinations so that dashboard keeps receiving fresh data until it's migrated to query the shared project.
|
Addressed the dashboard-continuity concern raised in review: this now dual-writes every event to both PostHog destinations instead of a hard cutover, so the existing heygen-cli dashboard keeps receiving fresh data rather than going dark as clients upgrade. New tests cover the fan-out and its failure isolation (one destination erroring doesn't block the other). |
3 participants
Summary
heygen-cli's own analytics reported to a separate PostHog project under a locally-minted identity, with no identify call on login — so a successful
heygen auth logincouldn't be joined to anything the rest of the toolchain already tracks, and login attempts only ever showed up as a generic command-run event with no method or failure reason. This wires heygen-cli into the same cross-tool identity and destination, and makes login itself first-class, queryable telemetry.Companion change to a matching PR in
heygen-com/hyperframes: heygen-com/hyperframes#2130 (media-use side: failure classification + free/paid tagging + test-isolation hardening). The two land independently — neither repo's release blocks the other.What changed
surfaceproperty so a query can still isolate this CLI's traffic.auth loginattempts now emit distinct started/completed/failed events carrying the method (OAuth vs. API-key) and, on failure, a specific reason drawn from the login flow's real failure branches (timeout, cancellation, invalid input, etc.) instead of a generic pass/fail.startedevent with no matchingcompleted/failed; every path now reconciles through a safety-net terminal event.Design decisions
Testing
go testrun; verified by careful manual reading and cross-checking against the pinned SDK's actual source.Post-Deploy Monitoring & Validation