Repository navigation
fix(media-use): honor telemetryEnabled: false from shared config in optedOut - #5490
Conversation
…ptedOut The CLI's 'hyperframes telemetry disable' writes telemetryEnabled: false to ~/.hyperframes/config.json, but the media-use scripts' optedOut() only checked env vars, so transcribe.mjs, transcript-cut.mjs and audio-duck.mjs kept sending PostHog events for users who had opted out through the CLI. optedOut() now also returns true when the shared config has telemetryEnabled === false, reusing the readSharedConfig() helper already in the same file. Applied byte-identically to both copies of telemetry.mjs (skills/media-use/scripts/lib and packages/cli/src/media-use/lib, per the copy-parity rule), with tests in both test files. Closes heygen-com#5465
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 836619ca. The fix itself is right, but two things in this PR need changing before it can merge.
The fix is correct.
readSharedConfig().telemetryEnabled === falsereads the file the CLI writes.packages/cli/src/telemetry/config.tswrites~/.hyperframes/config.json, andclient.tsreads the sametelemetryEnabledkey.readSharedConfignever throws: an unreadable or non-object file reads as{}.- All three senders check
optedOut()before doing anything (the batch flush,identifyAccountandtrack), sohyperframes telemetry disablenow also stops the account-email identify. - Both copies of
telemetry.mjsare still byte-identical.
Changes needed
skills-manifest.jsonisn't regenerated.bun packages/cli/scripts/gen-skills-manifest.ts --checkat this head reports~ media-use (d0875c7f8bc28d51 → …), so the required "Skills: manifest in sync" job will fail. Runbun run --cwd packages/cli gen:skills-manifestafter the change below and commit the result. The hash depends on the test file too.skills/media-use/scripts/lib/telemetry.test.mjswrites the real~/.hyperframes/config.json. It buildsconfigPathfromhomedir()and restores the saved contents intest.after.- In CI this happens to be safe, because
Test: skillsruns with--import ./packages/cli/scripts/test-home.mjs, which pointsHOMEat a temp folder. - A developer running
node --teston this file directly overwrites their own config. If the run dies beforeafter, they lose it, or end up withtelemetryEnabledset to whatever the last case wrote. - The
packages/clitest in this same PR does it the safe way withsandbox(). Do the same here: setHOME/USERPROFILEto amkdtempSyncfolder inbefore, buildconfigPathfrom that folder, and delete it inafter.os.homedir()readsHOMEon every call, soreadSharedConfig()follows.
- In CI this happens to be safe, because
With both done, the 5 skills cases and the 15 packages/cli cases pass, --check is in sync, and the real config isn't touched. I checked this locally with exactly those two changes.
— Rames
…erate the skills manifest Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts: # skills-manifest.json
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 0b07e51c. It's 836619ca plus two maintainer commits, pushed with your "allow edits" permission. 649aef15 makes the two changes from my review, and 0b07e51c merges main (86ff462d), which conflicted only in skills-manifest.json. The diff against main is the 5 files of this fix and nothing else.
telemetry.test.mjsno longer touches the real config.beforepointsHOME/USERPROFILEat amkdtempSyncfolder,configPathis built from that folder, andafterrestores the environment and deletes the folder. The test no longer reads, saves or rewrites~/.hyperframes/config.json.skills-manifest.jsonis regenerated on the merged tree: media-use is now0fc5a05cc62ef03b.
Locally:
- All 5 skills cases and 15
packages/clitelemetry cases pass. gen-skills-manifest.ts --checkis in sync.oxfmt --checkwith the repo config is clean.- A real
~/.hyperframes/config.jsonwas left untouched.
The fix itself is unchanged from my earlier review and correct: hyperframes telemetry disable now also stops media-use's track, flush and account identify. Thanks for the fix, @iamcipherdev.
— Rames
# Conflicts: # skills-manifest.json
# Conflicts: # skills-manifest.json
What
Media-use telemetry now honors the CLI opt-out:
optedOut()intelemetry.mjsalso returnstruewhen the shared config (~/.hyperframes/config.json) hastelemetryEnabled: false.Why
hyperframes telemetry disablewritestelemetryEnabled: falseto the shared config, but the media-use scripts'optedOut()only checked env vars (HYPERFRAMES_NO_TELEMETRY,DO_NOT_TRACK,CI,NODE_ENV). Sotranscribe.mjs,transcript-cut.mjs, andaudio-duck.mjskept posting PostHog events for a user who had opted out through the CLI — and when~/.heygen/credentialsexists,identifyAccount()additionally sends the account email as the distinct id.Related work
Fixes #5465
Checked for overlap before starting (per TRIAGE.md): no comments, assignees, or open PRs on this issue; the open telemetry PRs (#5425 onnxruntime telemetry, #5460 prefs serialization) touch different behavior.
How
optedOut()now checksreadSharedConfig().telemetryEnabled === falsefirst, reusing the helper already present in the same file (it never throws; an unreadable/malformed config is treated as empty, so tracking behavior is unchanged in that case).telemetry.mjs—skills/media-use/scripts/lib/andpackages/cli/src/media-use/lib/— per the media-use copy-parity rule (scripts/check-media-use-copy-parity.test.mjs); verified the two files are identical after the change.Test plan
New test in
packages/cli/src/media-use/lib/telemetry.test.mjs(sandboxed HOME: no config → allowed;telemetryEnabled: false→ opted out;true→ allowed; malformed JSON → allowed) and a newskills/media-use/scripts/lib/telemetry.test.mjscovering the same config cases plus the env opt-out on the standalone copy.Ran with the repo's test-home harness (
node --import ./packages/cli/scripts/test-home.mjs --test <files>): CLI telemetry suite 15/15 pass, skills telemetry suite 5/5 pass.oxfmt --checkandoxlintare clean on all four changed files. I worked from a sparse checkout, so I did not run the fullbun run testsuite.