From bb6726a323127e67bd9468c45132e4febd333e50 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Fri, 10 Jul 2026 02:08:04 -0400 Subject: [PATCH 1/6] feat(media-use): classify heygen provider failures and track by reason Adds a stable reason-code classifier (not_found/not_authenticated/ outdated/rate_limited/other) alongside the existing display-message classifier, and reports it via a new media_use_provider_error event so quota/rate-limit failures are distinguishable from other provider errors even when a fallback provider later succeeds. --- skills-manifest.json | 2 +- skills/media-use/scripts/lib/heygen-cli.mjs | 38 ++++- .../media-use/scripts/lib/heygen-cli.test.mjs | 144 ++++++++++++++++++ 3 files changed, 177 insertions(+), 7 deletions(-) diff --git a/skills-manifest.json b/skills-manifest.json index 82e22e1c56..c20f3f8ae7 100644 --- a/skills-manifest.json +++ b/skills-manifest.json @@ -46,7 +46,7 @@ "files": 10 }, "media-use": { - "hash": "f6f3af6648b1bd81", + "hash": "2a223e1a31b719f7", "files": 122 }, "motion-graphics": { diff --git a/skills/media-use/scripts/lib/heygen-cli.mjs b/skills/media-use/scripts/lib/heygen-cli.mjs index e3db8ceb2b..290d93e4a4 100644 --- a/skills/media-use/scripts/lib/heygen-cli.mjs +++ b/skills/media-use/scripts/lib/heygen-cli.mjs @@ -1,3 +1,5 @@ +import { track } from "./telemetry.mjs"; + // v0.3.0 is the first CLI that can use an OAuth session; v0.1.x/0.2.x reject it // ("heygen-cli can't use OAuth yet"), and OAuth is what the free-usage path // needs — so anything below this can't authenticate for free usage at all. @@ -20,6 +22,14 @@ const ACTIONABLE_MESSAGES = new Set([ ]); export function classifyHeygenError(err) { + return classifyHeygenErrorResult(err).message; +} + +export function classifyHeygenErrorCode(err) { + return classifyHeygenErrorResult(err).code; +} + +function classifyHeygenErrorResult(err) { const detail = heygenErrorDetail(err); const text = [err?.stderr, err?.stdout, err?.message, detail] .map((value) => textOf(value)) @@ -33,7 +43,7 @@ export function classifyHeygenError(err) { // embeds the `heygen ...` command line — sending users to reinstall a CLI they // just ran successfully. Keep this narrow. if (err?.code === "ENOENT" || lower.includes("command not found")) { - return HEYGEN_NOT_FOUND_MESSAGE; + return { code: "not_found", message: HEYGEN_NOT_FOUND_MESSAGE }; } if ( @@ -50,24 +60,40 @@ export function classifyHeygenError(err) { lower.includes("auth required") || lower.includes("authentication required") ) { - return HEYGEN_NOT_AUTHENTICATED_MESSAGE; + return { code: "not_authenticated", message: HEYGEN_NOT_AUTHENTICATED_MESSAGE }; } const version = firstSemver(text); if (version && versionLessThan(version, HEYGEN_MIN_VERSION)) { - return HEYGEN_OUTDATED_MESSAGE; + return { code: "outdated", message: HEYGEN_OUTDATED_MESSAGE }; } - return detail; + if ( + lower.includes("rate limit") || + lower.includes("quota") || + lower.includes("insufficient credit") || + /\b429\b/.test(lower) + ) { + return { code: "rate_limited", message: detail }; + } + + return { code: "other", message: detail }; } -export function reportHeygenFailure(err, context) { - const message = classifyHeygenError(err); +export function reportHeygenFailure(err, context, trackEvent = track) { + const { code, message } = classifyHeygenErrorResult(err); if (ACTIONABLE_MESSAGES.has(message)) { console.error(message); } else { console.error(`media-use: \`${context}\` failed: ${message}`); } + try { + void Promise.resolve( + trackEvent("media_use_provider_error", { provider: "heygen", reason: code }), + ).catch(() => {}); + } catch { + // Telemetry must never affect the provider failure path. + } } export function firstSemver(text) { diff --git a/skills/media-use/scripts/lib/heygen-cli.test.mjs b/skills/media-use/scripts/lib/heygen-cli.test.mjs index c58acd6d8e..28bd2cb5f5 100644 --- a/skills/media-use/scripts/lib/heygen-cli.test.mjs +++ b/skills/media-use/scripts/lib/heygen-cli.test.mjs @@ -1,12 +1,31 @@ import { strict as assert } from "node:assert"; +import { spawnSync } from "node:child_process"; import { test } from "node:test"; import { classifyHeygenError, + classifyHeygenErrorCode, HEYGEN_NOT_AUTHENTICATED_MESSAGE, HEYGEN_NOT_FOUND_MESSAGE, HEYGEN_OUTDATED_MESSAGE, + reportHeygenFailure, } from "./heygen-cli.mjs"; +function captureFailureReport(err, context, trackEvent) { + const originalError = console.error; + const stderrCalls = []; + console.error = (...args) => stderrCalls.push(args); + try { + if (trackEvent) { + reportHeygenFailure(err, context, trackEvent); + } else { + reportHeygenFailure(err, context); + } + } finally { + console.error = originalError; + } + return stderrCalls; +} + test("classifies ENOENT-style missing heygen errors with install instructions", () => { const message = classifyHeygenError({ code: "ENOENT", message: "spawn heygen ENOENT" }); @@ -64,3 +83,128 @@ test("passes through unrelated errors", () => { assert.equal(message, "rate limit exceeded"); }); + +test("classifies existing HeyGen failures with stable reason codes", () => { + assert.equal(classifyHeygenErrorCode({ code: "ENOENT" }), "not_found"); + assert.equal( + classifyHeygenErrorCode({ stderr: Buffer.from("HTTP 401 Unauthorized") }), + "not_authenticated", + ); + assert.equal( + classifyHeygenErrorCode({ stderr: Buffer.from("heygen v0.1.5 is unsupported") }), + "outdated", + ); + assert.equal(classifyHeygenErrorCode({ stderr: Buffer.from("provider unavailable") }), "other"); +}); + +test("classifies rate-limit text case-insensitively", () => { + assert.equal( + classifyHeygenErrorCode({ stderr: Buffer.from("RATE LIMIT exceeded") }), + "rate_limited", + ); +}); + +test("classifies quota and insufficient-credit errors as rate limited", () => { + for (const detail of ["Quota exhausted", "INSUFFICIENT CREDIT remaining"]) { + assert.equal(classifyHeygenErrorCode({ stderr: Buffer.from(detail) }), "rate_limited"); + } +}); + +test("classifies a bare 429 as rate limited without matching request IDs", () => { + assert.equal( + classifyHeygenErrorCode({ stderr: Buffer.from("HTTP 429 Too Many Requests") }), + "rate_limited", + ); + assert.equal( + classifyHeygenErrorCode({ stderr: Buffer.from("request req-429abc failed") }), + "other", + ); +}); + +test("tracks not-found failures without changing actionable output", () => { + const trackingCalls = []; + const stderrCalls = captureFailureReport({ code: "ENOENT" }, "heygen asset search", (...args) => + trackingCalls.push(args), + ); + + assert.deepEqual(stderrCalls, [[HEYGEN_NOT_FOUND_MESSAGE]]); + assert.deepEqual(trackingCalls, [ + ["media_use_provider_error", { provider: "heygen", reason: "not_found" }], + ]); +}); + +test("tracks generic failures without including raw detail", () => { + const trackingCalls = []; + const stderrCalls = captureFailureReport( + { stderr: Buffer.from("private provider detail") }, + "heygen asset search", + (...args) => trackingCalls.push(args), + ); + + assert.deepEqual(stderrCalls, [ + ["media-use: `heygen asset search` failed: private provider detail"], + ]); + assert.deepEqual(trackingCalls, [ + ["media_use_provider_error", { provider: "heygen", reason: "other" }], + ]); +}); + +test("keeps failure output observable when telemetry is opted out", () => { + const previousOptOut = process.env.HYPERFRAMES_NO_TELEMETRY; + process.env.HYPERFRAMES_NO_TELEMETRY = "1"; + try { + const stderrCalls = captureFailureReport({ code: "ENOENT" }, "heygen asset search"); + + assert.deepEqual(stderrCalls, [[HEYGEN_NOT_FOUND_MESSAGE]]); + } finally { + if (previousOptOut === undefined) { + delete process.env.HYPERFRAMES_NO_TELEMETRY; + } else { + process.env.HYPERFRAMES_NO_TELEMETRY = previousOptOut; + } + } +}); + +test("keeps failure output observable when tracking throws synchronously", () => { + const originalError = console.error; + const stderrCalls = []; + let thrown; + console.error = (...args) => stderrCalls.push(args); + try { + try { + reportHeygenFailure({ code: "ENOENT" }, "heygen asset search", () => { + throw new Error("tracking failed"); + }); + } catch (err) { + thrown = err; + } + } finally { + console.error = originalError; + } + + assert.deepEqual(stderrCalls, [[HEYGEN_NOT_FOUND_MESSAGE]]); + assert.equal(thrown, undefined); +}); + +test("does not leave rejected tracking promises unhandled", () => { + const moduleUrl = new URL("./heygen-cli.mjs", import.meta.url).href; + const script = ` + import { reportHeygenFailure } from ${JSON.stringify(moduleUrl)}; + reportHeygenFailure( + { stderr: "provider unavailable" }, + "heygen asset search", + () => Promise.reject(new Error("tracking failed")), + ); + await new Promise((resolve) => setImmediate(resolve)); + `; + const child = spawnSync( + process.execPath, + ["--unhandled-rejections=strict", "--input-type=module", "--eval", script], + { encoding: "utf8", timeout: 5000 }, + ); + + assert.equal(child.error, undefined); + assert.equal(child.signal, null); + assert.equal(child.status, 0, child.stderr); + assert.equal(child.stderr, "media-use: `heygen asset search` failed: provider unavailable\n"); +}); From 2f45c34435f5b56f616fc701260c0e371fa221c6 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Fri, 10 Jul 2026 02:12:13 -0400 Subject: [PATCH 2/6] feat(media-use): tag free vs paid auth path on heygen-backed resolves Adds a sparse auth_method property (oauth/api_key) to media_use_resolve telemetry for heygen-family providers, derived via a new heygenAuthMethod() helper alongside the existing heygenAuthHeaders(). Absent for every non-heygen provider. --- skills-manifest.json | 2 +- skills/media-use/audio/scripts/lib/heygen.mjs | 11 +++ .../audio/scripts/lib/heygen.test.mjs | 42 +++++++++- skills/media-use/scripts/resolve.mjs | 14 ++++ skills/media-use/scripts/resolve.test.mjs | 78 +++++++++++++++++++ 5 files changed, 145 insertions(+), 2 deletions(-) diff --git a/skills-manifest.json b/skills-manifest.json index c20f3f8ae7..347a9ae925 100644 --- a/skills-manifest.json +++ b/skills-manifest.json @@ -46,7 +46,7 @@ "files": 10 }, "media-use": { - "hash": "2a223e1a31b719f7", + "hash": "b99ad1530966d786", "files": 122 }, "motion-graphics": { diff --git a/skills/media-use/audio/scripts/lib/heygen.mjs b/skills/media-use/audio/scripts/lib/heygen.mjs index 77bcba10f5..e74614ec7b 100644 --- a/skills/media-use/audio/scripts/lib/heygen.mjs +++ b/skills/media-use/audio/scripts/lib/heygen.mjs @@ -69,6 +69,17 @@ export function heygenCredential() { return null; } +// → "oauth" | "api_key" | null. Same oauth-vs-api-key check heygenAuthHeaders() +// makes internally, exposed on its own so callers that only need to *tag* the +// auth path (telemetry) don't have to parse headers back apart. Never throws: +// no credential (or an expired one) is just `null`, same as a fresh resolve +// with nothing to tag. +export function heygenAuthMethod() { + const cred = heygenCredential(); + if (!cred?.headers) return null; + return "Authorization" in cred.headers ? "oauth" : "api_key"; +} + // → auth headers object, or throw with a fix hint. export function heygenAuthHeaders() { const cred = heygenCredential(); diff --git a/skills/media-use/audio/scripts/lib/heygen.test.mjs b/skills/media-use/audio/scripts/lib/heygen.test.mjs index 34672aee61..629a17fd15 100644 --- a/skills/media-use/audio/scripts/lib/heygen.test.mjs +++ b/skills/media-use/audio/scripts/lib/heygen.test.mjs @@ -3,7 +3,7 @@ import assert from "node:assert/strict"; import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { heygenAuthHeaders } from "./heygen.mjs"; +import { heygenAuthHeaders, heygenAuthMethod } from "./heygen.mjs"; function withCleanHeygenEnv(fn) { const previousApiKey = process.env.HEYGEN_API_KEY; @@ -58,3 +58,43 @@ test("heygenAuthHeaders tags OAuth requests as CLI traffic", () => { } }); }); + +test("heygenAuthMethod returns api_key for an env API key, without tagging headers", () => { + withCleanHeygenEnv(() => { + process.env.HEYGEN_API_KEY = "hg_test"; + assert.equal(heygenAuthMethod(), "api_key"); + }); +}); + +test("heygenAuthMethod returns oauth for a live OAuth credential", () => { + withCleanHeygenEnv(() => { + const dir = mkdtempSync(join(tmpdir(), "heygen-cred-")); + try { + process.env.HEYGEN_CONFIG_DIR = dir; + writeFileSync( + join(dir, "credentials"), + JSON.stringify({ + oauth: { + access_token: "at_test", + expires_at: "2099-01-01T00:00:00Z", + }, + }), + ); + assert.equal(heygenAuthMethod(), "oauth"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + +test("heygenAuthMethod returns null with no credential at all", () => { + withCleanHeygenEnv(() => { + const dir = mkdtempSync(join(tmpdir(), "heygen-cred-")); + try { + process.env.HEYGEN_CONFIG_DIR = dir; // no credentials file written + assert.equal(heygenAuthMethod(), null); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/skills/media-use/scripts/resolve.mjs b/skills/media-use/scripts/resolve.mjs index 1de70e7bf9..b1248b5875 100644 --- a/skills/media-use/scripts/resolve.mjs +++ b/skills/media-use/scripts/resolve.mjs @@ -16,6 +16,7 @@ import { buildStats } from "./lib/stats.mjs"; import { typesMatch } from "./lib/match.mjs"; import { listCandidates, formatCandidates, CANDIDATE_CAP } from "./lib/candidates.mjs"; import { findGlobalBySha } from "./lib/cache.mjs"; +import { heygenAuthMethod } from "../audio/scripts/lib/heygen.mjs"; import { buildCube, paramsFromIntent } from "./lib/cube-build.mjs"; import { validateCubeFile } from "./lib/cube-validate.mjs"; import { analyzeMediaGrade, formatMeasuredNote } from "./lib/grade-analyzer.mjs"; @@ -212,6 +213,15 @@ function recordAvailable(projectDir, record) { return record.type === "grade" && record.grading; } +// Sparse `{ authMethod }` for a heygen-family provider name (e.g. "heygen.tts"), +// else `{}` — keeps auth_method telemetry absent for every non-heygen resolve +// instead of implying an auth method that doesn't apply. +function heygenAuthMethodFor(provider) { + if (!provider || !provider.startsWith("heygen.")) return {}; + const authMethod = heygenAuthMethod(); + return authMethod ? { authMethod } : {}; +} + function localizeImportedRecord(record, localPath) { if (record?.type === "grade" && record.grading?.lut) { record.grading = { @@ -401,6 +411,7 @@ async function run() { provenance: { provider: searchResult.metadata?.provider || "unknown", prompt: intent, + ...heygenAuthMethodFor(searchResult.metadata?.provider), ...searchResult.metadata?.provenance, }, }; @@ -1056,6 +1067,9 @@ async function result(record, source) { // parametric), or "params" (offline). Surfaces silent CDN→params downgrades // in prod, which --doctor can't (it only answers "reachable now?"). via: record.provenance?.via, + // Free (OAuth) vs. paid (API-key) heygen path — sparse: absent for every + // non-heygen provider (see heygenAuthMethodFor at construction time). + auth_method: record.provenance?.authMethod, local_only: !!args["local-only"], provider_override: !!args.provider, }); diff --git a/skills/media-use/scripts/resolve.test.mjs b/skills/media-use/scripts/resolve.test.mjs index 2c4ff924a5..744750d908 100644 --- a/skills/media-use/scripts/resolve.test.mjs +++ b/skills/media-use/scripts/resolve.test.mjs @@ -179,6 +179,84 @@ test("entity hit matches across icon/image (figma-imported brand marks)", () => cleanup(); }); +// --- auth_method provenance (U6) --- + +test("manifest hit for an OAuth-credentialed heygen resolve surfaces authMethod: oauth", () => { + setup(); + const record = makeRecord({ + id: "voice_001", + type: "voice", + path: ".media/audio/voice/voice_001.wav", + provenance: { provider: "heygen.tts", authMethod: "oauth", prompt: "oauth voice" }, + }); + appendRecord(tmp, record); + const filePath = join(tmp, record.path); + mkdirSync(join(filePath, ".."), { recursive: true }); + writeFileSync(filePath, "cached voice"); + + const out = runResolve([ + "--type", + "voice", + "--intent", + "oauth voice", + "--project", + tmp, + "--json", + ]); + const parsed = JSON.parse(out.trim()); + assert.equal(parsed.ok, true); + assert.equal(parsed.provenance.authMethod, "oauth"); + cleanup(); +}); + +test("manifest hit for an API-key-credentialed heygen resolve surfaces authMethod: api_key", () => { + setup(); + const record = makeRecord({ + id: "voice_001", + type: "voice", + path: ".media/audio/voice/voice_001.wav", + provenance: { provider: "heygen.tts", authMethod: "api_key", prompt: "api key voice" }, + }); + appendRecord(tmp, record); + const filePath = join(tmp, record.path); + mkdirSync(join(filePath, ".."), { recursive: true }); + writeFileSync(filePath, "cached voice"); + + const out = runResolve([ + "--type", + "voice", + "--intent", + "api key voice", + "--project", + tmp, + "--json", + ]); + const parsed = JSON.parse(out.trim()); + assert.equal(parsed.ok, true); + assert.equal(parsed.provenance.authMethod, "api_key"); + cleanup(); +}); + +test("manifest hit for a non-heygen provider omits authMethod entirely", () => { + setup(); + const record = makeRecord({ + id: "logo_001", + type: "logo", + path: ".media/images/logo_001.svg", + provenance: { provider: "svgl", prompt: "acme logo" }, + }); + appendRecord(tmp, record); + const filePath = join(tmp, record.path); + mkdirSync(join(filePath, ".."), { recursive: true }); + writeFileSync(filePath, ""); + + const out = runResolve(["--type", "logo", "--intent", "acme logo", "--project", tmp, "--json"]); + const parsed = JSON.parse(out.trim()); + assert.equal(parsed.ok, true); + assert.equal("authMethod" in parsed.provenance, false); + cleanup(); +}); + // --- global cache hit --- test("global cache hit copies to project and registers", () => { From e2ed3c6d3f2ff49d4e023a80b647fc0066d762f3 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Fri, 10 Jul 2026 02:17:59 -0400 Subject: [PATCH 3/6] fix(media-use): harden test isolation for telemetry with a real interception seam resolve.test.mjs already set DO_NOT_TRACK=1 for every spawned resolve.mjs invocation, so local test runs were not actually leaking events. But that safety relied entirely on every call site remembering to set the env var, with no way to prove it. Add a MEDIA_USE_TELEMETRY_HOST override read at the point telemetry.mjs builds its POST url (falling back to the real endpoint when unset), and a test that points it at a local HTTP server to assert a real event is actually intercepted rather than trusting the env-var default alone. --- skills-manifest.json | 2 +- skills/media-use/scripts/lib/telemetry.mjs | 11 +++- skills/media-use/scripts/resolve.test.mjs | 76 ++++++++++++++++++++++ 3 files changed, 87 insertions(+), 2 deletions(-) diff --git a/skills-manifest.json b/skills-manifest.json index 347a9ae925..b430d96a46 100644 --- a/skills-manifest.json +++ b/skills-manifest.json @@ -46,7 +46,7 @@ "files": 10 }, "media-use": { - "hash": "b99ad1530966d786", + "hash": "7c3982d07b883fe1", "files": 122 }, "motion-graphics": { diff --git a/skills/media-use/scripts/lib/telemetry.mjs b/skills/media-use/scripts/lib/telemetry.mjs index eefcdc346a..9652bbc9a3 100644 --- a/skills/media-use/scripts/lib/telemetry.mjs +++ b/skills/media-use/scripts/lib/telemetry.mjs @@ -21,6 +21,15 @@ const POSTHOG_HOST = "https://us.i.posthog.com"; const TIMEOUT_MS = 1500; let identifiedAccount = false; +// Test-only interception seam: a real HTTP destination a test can point at, +// so a spawned-child test (resolve.test.mjs) can prove track() never reaches +// production rather than trusting DO_NOT_TRACK alone (a future call site or +// test could forget to set that env var). Falls back to the real production +// host whenever unset — production behavior is unchanged. +function posthogHost() { + return process.env.MEDIA_USE_TELEMETRY_HOST || POSTHOG_HOST; +} + /** True when telemetry must NOT be sent (opt-out envs, CI, dev). */ export function optedOut() { return ( @@ -131,7 +140,7 @@ function showTelemetryNotice() { async function postBatch(batch) { try { - await fetch(`${POSTHOG_HOST}/batch/`, { + await fetch(`${posthogHost()}/batch/`, { method: "POST", headers: { "Content-Type": "application/json", Connection: "close" }, body: JSON.stringify({ api_key: POSTHOG_API_KEY, batch }), diff --git a/skills/media-use/scripts/resolve.test.mjs b/skills/media-use/scripts/resolve.test.mjs index 744750d908..706d4b22ba 100644 --- a/skills/media-use/scripts/resolve.test.mjs +++ b/skills/media-use/scripts/resolve.test.mjs @@ -10,6 +10,7 @@ import { } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; +import { createServer } from "node:http"; import { execFileSync, spawnSync } from "node:child_process"; import { appendRecord, readManifest } from "./lib/manifest.mjs"; import { regenerateIndex } from "./lib/index-gen.mjs"; @@ -687,6 +688,81 @@ test("identical grade resolve hits the project cache without re-freezing", () => cleanup(); }); +// --- telemetry isolation (U7) --- + +// Every other test relies on runResolve/spawnResolve's default DO_NOT_TRACK: +// "1" to keep track() a no-op. That default is fragile on its own (a future +// call site or test could forget to set it), so telemetry.mjs also exposes a +// MEDIA_USE_TELEMETRY_HOST override read at the point the POST URL is built. +// This test proves that seam actually intercepts a real event end to end: a +// resolve that reaches track("media_use_resolve", ...) with tracking allowed +// posts to a local HTTP server instead of production, and the server actually +// receives it (not just "nothing happened because nothing was listening"). +test("track() posts to MEDIA_USE_TELEMETRY_HOST when set, proving real interception", async () => { + setup(); + const received = []; + const server = createServer((req, res) => { + let body = ""; + req.on("data", (chunk) => (body += chunk)); + req.on("end", () => { + try { + received.push(JSON.parse(body)); + } catch { + // ignore malformed body; assertions below fail on empty `received` + } + res.writeHead(200, { "Content-Type": "application/json" }); + res.end("{}"); + }); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const port = server.address().port; + const sandboxHome = mkdtempSync(join(tmpdir(), "mu-resolve-telemetry-home-")); + + try { + const record = makeRecord({ + provenance: { prompt: "telemetry seam test", provider: "test" }, + }); + appendRecord(tmp, record); + const filePath = join(tmp, record.path); + mkdirSync(join(filePath, ".."), { recursive: true }); + writeFileSync(filePath, "telemetry seam audio"); + + // Override this one invocation's env only: allow tracking (DO_NOT_TRACK + // default flipped off), sandbox HOME so anonymousId()/showTelemetryNotice() + // never touch the real developer machine, and point the host at the local + // server. Every other test in this file keeps its untouched default env. + runResolve(["--type", "bgm", "--intent", "telemetry seam test", "--project", tmp, "--json"], { + env: { + DO_NOT_TRACK: "0", + HYPERFRAMES_NO_TELEMETRY: "0", + CI: "", + NODE_ENV: "test", + HOME: sandboxHome, + MEDIA_USE_TELEMETRY_HOST: `http://127.0.0.1:${port}`, + }, + }); + + // runResolve blocks synchronously (execFileSync) until the child exits, which + // pauses this process's own event loop for that whole span — the child's + // request to our local server sits accepted-but-unprocessed in the kernel + // backlog until control returns here. Poll briefly to let the event loop + // drain it rather than asserting before the server has had a turn to run. + for (let i = 0; i < 100 && received.length === 0; i++) { + await new Promise((resolve) => setTimeout(resolve, 20)); + } + } finally { + await new Promise((resolve) => server.close(resolve)); + rmSync(sandboxHome, { recursive: true, force: true }); + cleanup(); + } + + assert.ok(received.length > 0, "expected the local telemetry server to receive a POST"); + const resolveEvent = received[0].batch.find((event) => event.event === "media_use_resolve"); + assert.ok(resolveEvent, "expected a media_use_resolve event in the intercepted batch"); + assert.equal(resolveEvent.properties.provider, "test"); + assert.equal(resolveEvent.properties.type, "bgm"); +}); + // --- run --- async function main() { From b398ef297eb8bea3628a2039724e10713ea6875d Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Fri, 10 Jul 2026 14:27:20 -0400 Subject: [PATCH 4/6] fix(media-use): harden telemetry host override and rate-limit classifier Warn once on stderr if MEDIA_USE_TELEMETRY_HOST is ever set outside a test/CI context, since the existing catch{} around the network call would otherwise swallow a silent redirect with zero signal. Also widen the rate_limited classifier to catch "too many requests" (the literal HTTP 429 reason phrase) and "throttled", which were falling through to "other". --- skills/media-use/scripts/lib/heygen-cli.mjs | 2 + .../media-use/scripts/lib/heygen-cli.test.mjs | 15 +++++ skills/media-use/scripts/lib/telemetry.mjs | 29 ++++++++- .../media-use/scripts/lib/telemetry.test.mjs | 59 +++++++++++++++++++ 4 files changed, 104 insertions(+), 1 deletion(-) diff --git a/skills/media-use/scripts/lib/heygen-cli.mjs b/skills/media-use/scripts/lib/heygen-cli.mjs index 290d93e4a4..9736e989df 100644 --- a/skills/media-use/scripts/lib/heygen-cli.mjs +++ b/skills/media-use/scripts/lib/heygen-cli.mjs @@ -72,6 +72,8 @@ function classifyHeygenErrorResult(err) { lower.includes("rate limit") || lower.includes("quota") || lower.includes("insufficient credit") || + lower.includes("too many requests") || + lower.includes("throttled") || /\b429\b/.test(lower) ) { return { code: "rate_limited", message: detail }; diff --git a/skills/media-use/scripts/lib/heygen-cli.test.mjs b/skills/media-use/scripts/lib/heygen-cli.test.mjs index 28bd2cb5f5..4467ce2a21 100644 --- a/skills/media-use/scripts/lib/heygen-cli.test.mjs +++ b/skills/media-use/scripts/lib/heygen-cli.test.mjs @@ -110,6 +110,21 @@ test("classifies quota and insufficient-credit errors as rate limited", () => { } }); +test("classifies the literal 429 reason phrase and throttling language as rate limited", () => { + for (const detail of ["Too Many Requests", "Error: throttled by upstream, retry later"]) { + assert.equal(classifyHeygenErrorCode({ stderr: Buffer.from(detail) }), "rate_limited"); + } +}); + +test("does not misclassify unrelated errors that share a word with the new phrasing", () => { + // Shares "too many" with "too many requests" but is a distinct failure (fd + // exhaustion, not a rate limit) — the match must require the full phrase. + assert.equal( + classifyHeygenErrorCode({ stderr: Buffer.from("Too many open file descriptors") }), + "other", + ); +}); + test("classifies a bare 429 as rate limited without matching request IDs", () => { assert.equal( classifyHeygenErrorCode({ stderr: Buffer.from("HTTP 429 Too Many Requests") }), diff --git a/skills/media-use/scripts/lib/telemetry.mjs b/skills/media-use/scripts/lib/telemetry.mjs index 9652bbc9a3..e5ad5227bc 100644 --- a/skills/media-use/scripts/lib/telemetry.mjs +++ b/skills/media-use/scripts/lib/telemetry.mjs @@ -20,14 +20,40 @@ const POSTHOG_API_KEY = "phc_zjjbX0PnWxERXrMHhkEJWj9A9BhGVLRReICgsfTMmpx"; const POSTHOG_HOST = "https://us.i.posthog.com"; const TIMEOUT_MS = 1500; let identifiedAccount = false; +let warnedNonDefaultHost = false; + +// Same CI/test signals the test suite itself sets (resolve.test.mjs's U7 test +// sets NODE_ENV=test and clears CI to prove the interception seam works) — +// reused here, not a new heuristic, so that deliberate test usage never +// triggers the warning below. +function isTestOrCiContext() { + return ( + process.env.CI === "true" || + process.env.CI === "1" || + process.env.NODE_ENV === "test" || + process.env.NODE_ENV === "development" + ); +} // Test-only interception seam: a real HTTP destination a test can point at, // so a spawned-child test (resolve.test.mjs) can prove track() never reaches // production rather than trusting DO_NOT_TRACK alone (a future call site or // test could forget to set that env var). Falls back to the real production // host whenever unset — production behavior is unchanged. +// +// Safety net: if this ever leaks into a real user's shell, track() would +// silently redirect to a likely-dead host and postBatch()'s catch{} would +// swallow the failure with zero signal. Surface one stderr warning outside +// test/CI contexts so a real user gets some indication instead of silence. function posthogHost() { - return process.env.MEDIA_USE_TELEMETRY_HOST || POSTHOG_HOST; + const override = process.env.MEDIA_USE_TELEMETRY_HOST; + if (override && !warnedNonDefaultHost && !isTestOrCiContext()) { + warnedNonDefaultHost = true; + console.error( + `media-use: telemetry is redirected to a non-default host via MEDIA_USE_TELEMETRY_HOST (${override}) — unset it unless this is intentional.`, + ); + } + return override || POSTHOG_HOST; } /** True when telemetry must NOT be sent (opt-out envs, CI, dev). */ @@ -189,4 +215,5 @@ export function __anonymousIdForTest() { export function __resetTelemetryForTest() { identifiedAccount = false; + warnedNonDefaultHost = false; } diff --git a/skills/media-use/scripts/lib/telemetry.test.mjs b/skills/media-use/scripts/lib/telemetry.test.mjs index fa61997bb1..f3d9feb4c9 100644 --- a/skills/media-use/scripts/lib/telemetry.test.mjs +++ b/skills/media-use/scripts/lib/telemetry.test.mjs @@ -251,6 +251,65 @@ test("first run notice prints to stderr once and never stdout", async () => { } }); +test("warns once on stderr when MEDIA_USE_TELEMETRY_HOST is set outside a test/CI context", async () => { + const savedEnv = { ...process.env }; + const originalFetch = globalThis.fetch; + const originalError = console.error; + const { root } = sandbox(); + const stderr = []; + globalThis.fetch = async () => ({ ok: true }); + console.error = (...args) => stderr.push(args.join(" ")); + try { + // No opt-out, no CI/NODE_ENV signal — mirrors a real user's shell where + // this test-only var leaked in by accident and tracking is not disabled. + withoutTelemetryOptOut(); + process.env.MEDIA_USE_TELEMETRY_HOST = "http://127.0.0.1:1"; // nothing listens; irrelevant here + await track("media_use_resolve", { type: "bgm" }); + await track("media_use_resolve", { type: "bgm" }); + + const warnings = stderr.filter((line) => line.includes("MEDIA_USE_TELEMETRY_HOST")); + assert.equal(warnings.length, 1, "expected exactly one warning across repeated calls"); + } finally { + globalThis.fetch = originalFetch; + console.error = originalError; + restoreEnv(savedEnv); + rmSync(root, { recursive: true, force: true }); + __resetTelemetryForTest(); + } +}); + +test("does not warn when MEDIA_USE_TELEMETRY_HOST is set the way the U7 interception test sets it", async () => { + const savedEnv = { ...process.env }; + const originalFetch = globalThis.fetch; + const originalError = console.error; + const { root } = sandbox(); + const stderr = []; + globalThis.fetch = async () => ({ ok: true }); + console.error = (...args) => stderr.push(args.join(" ")); + try { + withoutTelemetryOptOut(); + // Same env shape resolve.test.mjs's U7 test uses to allow tracking through: + // DO_NOT_TRACK=0, CI unset/empty, NODE_ENV=test. + process.env.DO_NOT_TRACK = "0"; + process.env.CI = ""; + process.env.NODE_ENV = "test"; + process.env.MEDIA_USE_TELEMETRY_HOST = "http://127.0.0.1:1"; + await track("media_use_resolve", { type: "bgm" }); + + assert.equal( + stderr.some((line) => line.includes("MEDIA_USE_TELEMETRY_HOST")), + false, + "the U7 test's own use of the override must not print a spurious warning", + ); + } finally { + globalThis.fetch = originalFetch; + console.error = originalError; + restoreEnv(savedEnv); + rmSync(root, { recursive: true, force: true }); + __resetTelemetryForTest(); + } +}); + test("read-only telemetry state degrades without throwing", async () => { const savedEnv = { ...process.env }; const originalFetch = globalThis.fetch; From 76409f06962d431aea37837c7f48e03d03e9131f Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Fri, 10 Jul 2026 15:00:19 -0400 Subject: [PATCH 5/6] fix(media-use): flush provider-error tracking before process exit reportHeygenFailure's telemetry call was fire-and-forget with no join point, and its callers (voice-provider.mjs, heygen-search.mjs) are sync call sites that can't await it themselves. On the resolve-miss path, process.exit() could follow shortly after, racing the still-in-flight request on its own non-keepalive connection and dropping the event. Track each pending report in a module-level set and flush it before the miss/success telemetry a process.exit() may follow. --- skills/media-use/scripts/lib/heygen-cli.mjs | 23 +++++++++- .../media-use/scripts/lib/heygen-cli.test.mjs | 42 +++++++++++++++++++ skills/media-use/scripts/resolve.mjs | 9 ++++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/skills/media-use/scripts/lib/heygen-cli.mjs b/skills/media-use/scripts/lib/heygen-cli.mjs index 9736e989df..d5208c9503 100644 --- a/skills/media-use/scripts/lib/heygen-cli.mjs +++ b/skills/media-use/scripts/lib/heygen-cli.mjs @@ -82,6 +82,14 @@ function classifyHeygenErrorResult(err) { return { code: "other", message: detail }; } +// reportHeygenFailure's callers (voice-provider.mjs, heygen-search.mjs) are +// synchronous and several layers below the CLI's process.exit() calls, so +// they can't await this tracking call themselves. Stash each attempt's +// promise here so a caller closer to exit (resolve.mjs) can join it first — +// same "awaited so a short-lived run flushes it" discipline telemetry.mjs's +// track() already documents, just reachable from a sync call site. +const pendingFailureTracking = new Set(); + export function reportHeygenFailure(err, context, trackEvent = track) { const { code, message } = classifyHeygenErrorResult(err); if (ACTIONABLE_MESSAGES.has(message)) { @@ -90,14 +98,27 @@ export function reportHeygenFailure(err, context, trackEvent = track) { console.error(`media-use: \`${context}\` failed: ${message}`); } try { - void Promise.resolve( + const tracked = Promise.resolve( trackEvent("media_use_provider_error", { provider: "heygen", reason: code }), ).catch(() => {}); + pendingFailureTracking.add(tracked); + void tracked.finally(() => pendingFailureTracking.delete(tracked)); + return tracked; } catch { // Telemetry must never affect the provider failure path. + return Promise.resolve(); } } +// Awaits every provider-error track fired since the last flush, so a caller +// about to process.exit() doesn't orphan one mid-request (both are separate, +// non-keepalive HTTP connections with no ordering guarantee otherwise). +// Never rejects: each tracked promise already swallows its own failure. +export async function flushHeygenFailureTracking() { + if (pendingFailureTracking.size === 0) return; + await Promise.all(pendingFailureTracking); +} + export function firstSemver(text) { const match = String(text || "").match(/\bv?(\d+)\.(\d+)\.(\d+)\b/); return match ? `${match[1]}.${match[2]}.${match[3]}` : null; diff --git a/skills/media-use/scripts/lib/heygen-cli.test.mjs b/skills/media-use/scripts/lib/heygen-cli.test.mjs index 4467ce2a21..04886c14dd 100644 --- a/skills/media-use/scripts/lib/heygen-cli.test.mjs +++ b/skills/media-use/scripts/lib/heygen-cli.test.mjs @@ -4,6 +4,7 @@ import { test } from "node:test"; import { classifyHeygenError, classifyHeygenErrorCode, + flushHeygenFailureTracking, HEYGEN_NOT_AUTHENTICATED_MESSAGE, HEYGEN_NOT_FOUND_MESSAGE, HEYGEN_OUTDATED_MESSAGE, @@ -223,3 +224,44 @@ test("does not leave rejected tracking promises unhandled", () => { assert.equal(child.status, 0, child.stderr); assert.equal(child.stderr, "media-use: `heygen asset search` failed: provider unavailable\n"); }); + +test("flushHeygenFailureTracking waits for a pending report before resolving", async () => { + const events = []; + let releaseTrack; + const gate = new Promise((resolve) => { + releaseTrack = resolve; + }); + + // Mirrors the real call sites (voice-provider.mjs, heygen-search.mjs): + // fire-and-forget, the return value is never awaited by the caller. + reportHeygenFailure({ code: "ENOENT" }, "heygen voice speech", () => + gate.then(() => { + events.push("track-settled"); + }), + ); + + const flushed = flushHeygenFailureTracking().then(() => { + events.push("flush-resolved"); + }); + + // Let several pending microtasks drain before releasing the gate, so this + // proves flush is genuinely still waiting on the tracked promise -- not + // merely that it hasn't had a tick yet. + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + assert.deepEqual(events, [], "flush must not resolve while the tracked promise is still pending"); + + releaseTrack(); + await flushed; + + assert.deepEqual( + events, + ["track-settled", "flush-resolved"], + "flush must resolve only after the pending track settles, in that order", + ); +}); + +test("flushHeygenFailureTracking resolves immediately when nothing is pending", async () => { + await flushHeygenFailureTracking(); +}); diff --git a/skills/media-use/scripts/resolve.mjs b/skills/media-use/scripts/resolve.mjs index b1248b5875..8f3b876d76 100644 --- a/skills/media-use/scripts/resolve.mjs +++ b/skills/media-use/scripts/resolve.mjs @@ -31,6 +31,7 @@ import { HEYGEN_MIN_VERSION, HEYGEN_UPDATE_COMMAND, firstSemver, + flushHeygenFailureTracking, versionLessThan, } from "./lib/heygen-cli.mjs"; @@ -350,6 +351,14 @@ async function run() { } } + // A search/generate attempt against heygen may have fired a fire-and-forget + // media_use_provider_error track (reportHeygenFailure — heygen-search.mjs / + // voice-provider.mjs are sync call sites several layers below here and can't + // await it themselves). Join it now, before any process.exit() below can + // race it: both it and the miss/success telemetry below are separate, + // non-keepalive HTTP connections with no ordering guarantee otherwise. + await flushHeygenFailureTracking(); + if (!searchResult) { await track("media_use_resolve_miss", { type, From f0143f269c4c7f0a299b946b34fa622a6534d8e8 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Fri, 10 Jul 2026 16:22:35 -0400 Subject: [PATCH 6/6] fix(media-use): regenerate skills manifest, lowercase identify email, harden test isolation - skills-manifest.json was stale after the last commit, failing the manifest-in-sync CI check. - heygenAccountDistinctId() now lowercases the email/username it returns, so it joins with heygen-cli's own identify call regardless of the account's stored casing -- an uppercase-email user would otherwise split across two PostHog profiles. - The telemetry-interception test now sandboxes HEYGEN_CONFIG_DIR alongside HOME, so a developer with that var pointing at real credentials can't have their real email read into the test's local server payload. - Two clarifying comments: the provenance spread order in resolve.mjs, and why a cache-hit's auth_method reports the original fetch's credential, not the current resolve's. --- skills-manifest.json | 2 +- skills/media-use/scripts/lib/telemetry.mjs | 5 ++- .../media-use/scripts/lib/telemetry.test.mjs | 39 +++++++++++++++++++ skills/media-use/scripts/resolve.mjs | 10 ++++- skills/media-use/scripts/resolve.test.mjs | 8 +++- 5 files changed, 60 insertions(+), 4 deletions(-) diff --git a/skills-manifest.json b/skills-manifest.json index b430d96a46..10fb15d3c9 100644 --- a/skills-manifest.json +++ b/skills-manifest.json @@ -46,7 +46,7 @@ "files": 10 }, "media-use": { - "hash": "7c3982d07b883fe1", + "hash": "49aa5adae0800403", "files": 122 }, "motion-graphics": { diff --git a/skills/media-use/scripts/lib/telemetry.mjs b/skills/media-use/scripts/lib/telemetry.mjs index e5ad5227bc..43603d7629 100644 --- a/skills/media-use/scripts/lib/telemetry.mjs +++ b/skills/media-use/scripts/lib/telemetry.mjs @@ -139,7 +139,10 @@ function heygenAccountDistinctId() { const user = parsed.user; if (!user || typeof user !== "object" || Array.isArray(user)) return null; const id = typeof user.email === "string" && user.email.trim() ? user.email : user.username; - return typeof id === "string" && id.trim() ? id.trim() : null; + // Lowercased so this joins with the CLI's own identify call regardless of + // the account's stored email casing — two different-case distinct ids + // would otherwise split one person across two PostHog profiles. + return typeof id === "string" && id.trim() ? id.trim().toLowerCase() : null; } catch { return null; } diff --git a/skills/media-use/scripts/lib/telemetry.test.mjs b/skills/media-use/scripts/lib/telemetry.test.mjs index f3d9feb4c9..1a66d60f78 100644 --- a/skills/media-use/scripts/lib/telemetry.test.mjs +++ b/skills/media-use/scripts/lib/telemetry.test.mjs @@ -182,6 +182,45 @@ test("track identifies a signed-in HeyGen account once and still sends events", } }); +test("track identifies with a lowercased email regardless of stored casing", async () => { + const savedEnv = { ...process.env }; + const originalFetch = globalThis.fetch; + const { root, home } = sandbox(); + const calls = []; + globalThis.fetch = async (url, options) => { + calls.push({ url, options }); + return { ok: true }; + }; + try { + withoutTelemetryOptOut(); + mkdirSync(join(home, ".hyperframes"), { recursive: true }); + writeFileSync( + join(home, ".hyperframes/config.json"), + JSON.stringify({ anonymousId: "anon-3" }), + ); + mkdirSync(join(home, ".heygen"), { recursive: true }); + writeFileSync( + join(home, ".heygen/credentials"), + JSON.stringify({ user: { email: "Alice@Example.com", username: "alice" } }), + ); + + await track("media_use_resolve", { type: "bgm", source: "search" }); + + const batch = parseFetchBodies(calls); + const identify = batch.filter((item) => item.event === "$identify"); + assert.equal(identify.length, 1); + // Lowercased so this joins with heygen-cli's own identify call regardless + // of the account's stored email casing -- otherwise the same person could + // split into two PostHog profiles by email case alone. + assert.equal(identify[0].distinct_id, "alice@example.com"); + } finally { + globalThis.fetch = originalFetch; + restoreEnv(savedEnv); + rmSync(root, { recursive: true, force: true }); + __resetTelemetryForTest(); + } +}); + test("track does not identify when signed out", async () => { const savedEnv = { ...process.env }; const originalFetch = globalThis.fetch; diff --git a/skills/media-use/scripts/resolve.mjs b/skills/media-use/scripts/resolve.mjs index 8f3b876d76..617443ac70 100644 --- a/skills/media-use/scripts/resolve.mjs +++ b/skills/media-use/scripts/resolve.mjs @@ -420,6 +420,10 @@ async function run() { provenance: { provider: searchResult.metadata?.provider || "unknown", prompt: intent, + // heygenAuthMethodFor spreads first so an explicit authMethod on a + // future provider's own metadata.provenance can still override it below + // -- safe today (no provider sets authMethod itself), but keep this + // ordering if that ever changes. ...heygenAuthMethodFor(searchResult.metadata?.provider), ...searchResult.metadata?.provenance, }, @@ -1077,7 +1081,11 @@ async function result(record, source) { // in prod, which --doctor can't (it only answers "reachable now?"). via: record.provenance?.via, // Free (OAuth) vs. paid (API-key) heygen path — sparse: absent for every - // non-heygen provider (see heygenAuthMethodFor at construction time). + // non-heygen provider (see heygenAuthMethodFor at construction time). On a + // cache/reuse hit this reports how the asset was ORIGINALLY fetched, not + // this resolve's own credential state — intentional: it's a conversion + // signal about the fetch that actually consumed a heygen credit, not + // about the (free, no-credential) act of copying a cached file. auth_method: record.provenance?.authMethod, local_only: !!args["local-only"], provider_override: !!args.provider, diff --git a/skills/media-use/scripts/resolve.test.mjs b/skills/media-use/scripts/resolve.test.mjs index 706d4b22ba..fea0e3be8c 100644 --- a/skills/media-use/scripts/resolve.test.mjs +++ b/skills/media-use/scripts/resolve.test.mjs @@ -730,7 +730,12 @@ test("track() posts to MEDIA_USE_TELEMETRY_HOST when set, proving real intercept // Override this one invocation's env only: allow tracking (DO_NOT_TRACK // default flipped off), sandbox HOME so anonymousId()/showTelemetryNotice() // never touch the real developer machine, and point the host at the local - // server. Every other test in this file keeps its untouched default env. + // server. HEYGEN_CONFIG_DIR is sandboxed too -- runResolve's env is + // {...process.env, ...env}, so a developer with that var set to a real + // credentials dir would otherwise have heygenAccountDistinctId() read + // their real email into this test's local-server payload despite HOME + // being sandboxed (HEYGEN_CONFIG_DIR, not HOME, resolves the credentials + // path). Every other test in this file keeps its untouched default env. runResolve(["--type", "bgm", "--intent", "telemetry seam test", "--project", tmp, "--json"], { env: { DO_NOT_TRACK: "0", @@ -738,6 +743,7 @@ test("track() posts to MEDIA_USE_TELEMETRY_HOST when set, proving real intercept CI: "", NODE_ENV: "test", HOME: sandboxHome, + HEYGEN_CONFIG_DIR: join(sandboxHome, ".heygen"), MEDIA_USE_TELEMETRY_HOST: `http://127.0.0.1:${port}`, }, });