feat(media-use): fast heygen CLI onboarding — actionable diagnostics, --doctor, free-usage framing#2065
Conversation
5fa2c12 to
928539e
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
928539e to
8465bb5
Compare
8465bb5 to
45ee90e
Compare
45ee90e to
105bc16
Compare
105bc16 to
bc5086e
Compare
bc5086e to
2c9425c
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 2c9425c.
Read this one at the canonical / test-coverage / observability / cross-cutting angle (leaving HF-runtime-interop mechanics to Via + Miga). The --doctor shape is nice and the SKILL reframe reads clean; my findings are on the classifier's substring rules and one exit-code / doc divergence — I think one of them is a genuine blocker for a PR whose whole thesis is "no more dead-end diagnostics".
Blockers
skills/media-use/scripts/lib/heygen-cli.mjs:27-31—lower.includes("not found")will misclassify domain "not found" errors as CLI-not-installed, reintroducing the exact dead-end this PR is designed to eliminate.classifyHeygenErrorcatches an error from a successfully invoked heygen binary and then decides "installed?" from stderr substring alone. Concrete case:voice-provider.mjs:62callsheygen voice speech create --voice-id <id>; if the caller passes a stale/invalidvoiceId, the CLI's most-idiomatic error phrasing isvoice not found(orvoice with id X not found). That hitslower.includes("not found")and we printmedia-use: heygen CLI not found — Install: curl -fsSL .... The user, who obviously already has the CLI (it just ran and errored), installs it again, retries, same error — same dead-end loop the PR aims to break.heygen asset searchwith an invalid ID andheygen voice listreferencing a missing engine are the same shape. Fix: drop the bare"not found"clause; keeperr?.code === "ENOENT"(system-level: binary not spawnable) andlower.includes("command not found")(shell wrapper's own not-found line). Add a defensive test likeclassifyHeygenError({ stderr: "voice not found" })must NOT returnHEYGEN_NOT_FOUND_MESSAGE— otherwise the next refactor slides back in silently. (My memory has a matching pattern undersubstring_match_numeric_code_in_freeform_msg— same class of hazard applied to text codes here.)
Concerns
-
skills/media-use/scripts/resolve.mjs:814— top-levelokisffmpegonly, butSKILL.md(this PR, line 178) claimsffmpeg/ffprobeare strictly-required. Two sources of truth diverge. If any downstream script gates onresolve --doctorexit code to preflight a build (which is the obvious--doctorcontract), a missingffprobebox passes the gate and the build breaks at firstffprobecall. The PR body says "ffmpeg (the only strictly-required dep)" but the SKILL.md diff includes ffprobe. Pick one: either widen the gate toffmpeg && ffprobe(matches SKILL.md), or narrow the SKILL.md line to "ffmpegis strictly required;ffprobeis required for probe operations" (matches code). Also — the--doctor --jsontest atresolve.test.mjs:632-633enshrines the ffmpeg-only gate as intended, so if the doc is right and the code is wrong, the test will need to move first. -
skills/media-use/scripts/lib/heygen-cli.mjs:36—lower.includes("401")matches any freeform text containing the literal substring "401". Request IDs (req-401abc), URLs, retry-after headers, error codes for unrelated CLIs downstream of heygen (ffmpegpiped through, etc.) all trip this. Tighten to a word/whitespace boundary or a status-code shape (\b401\bregex, orHTTP 401/status: 401/\s401\s) so genuine 401 responses classify but noise doesn't. -
skills/media-use/scripts/resolve.mjs:849-862—emailFromAuthStatusregex captures the first@-shaped token in any prose. Ifheygen auth statusreturns success with a body likeSession expired. Contact support@heygen.ai to re-auth.,emailFromAuthStatusreturnssupport@heygen.aiand--doctorprints✓ heygen authenticated as support@heygen.ai — free-usage path: bgm/image/voice/avatar-video, misreporting an unauthenticated state as authenticated. Suggest gating on a preamble (/(logged in|authenticated) as\s+([^\s@]+@[^\s@]+\.[^\s@]+)/i) or — better — passing--jsonexplicitly toheygen auth statusand taking the JSON path only. The human-readable fallback is already a bet on a CLI-version-stable format; passing--jsonremoves the bet. -
No telemetry on
--doctorinvocation or per-check outcome. The PR's stated value is "dead ends become actionable install steps"; the way to know if that landed is instrumenting the exact question — how often are agents hitting--doctor, which check fails most, do users re-run after a fix. There's no signal emitted from any of the new code paths (no PostHog capture, no analytics breadcrumb, no metric). Not a blocker for landing, but this is the moment where adding a minimalcapture("media_use_doctor_run", { checks_failed })is trivial and later becomes a schema-migration to bolt on. Flagging so it doesn't become the "we shipped it and never learned anything from it" outcome. -
skills/media-use/scripts/resolve.mjs:765,781—heygen auth statusruns with no--json, no timeout distinction between slow-network and unauth.runCommanduses a shared 15s ceiling; if the auth endpoint stalls (transient network / DNS), status !== 0 and--doctorreports "heygen not authenticated" with fixheygen auth login --key <key>. Wrong actionable — the user re-logs in fine and the underlying network issue persists. Consider a distinct classifier for "probe timeout / spawn error" vs "authoritative unauth" (e.g. spawnSyncresult.error?.code === 'ETIMEDOUT'), or at minimum a— possible network issue if slowsoftener whenheygen --versionsucceeded butauth statustimed out.
Nits
skills/media-use/scripts/lib/heygen-cli.mjs:3—HEYGEN_INSTALL_COMMANDuses"...install.sh | bash then heygen auth login..."(double-space +then+ double-space) so the fix line renders as one visually-ambiguous command. Prefer... | bash && heygen auth login --key <key>(real shell chain, cut-and-pasteable) or split into twofixfields. (nit)skills/media-use/scripts/resolve.test.mjs:121-123—runResolveStatusis a straight alias forspawnResolvewith no added behavior; either drop it or inline. (nit)skills/media-use/scripts/resolve.mjs:765,781— passing["auth", "status", "--json"]explicitly (mirroring the JSON-parse branch you already have inemailFromAuthStatus) removes the human-format fragility acrossheygenversion bumps. (nit / defense-in-depth)
Questions
- Cross-PR with #2041: the color-grading types (
grade,lut) are documented as local-only in the SKILL.md diff (local core-preset map, params/CDN look index, deterministic buildCube fallback). Confirming: nograde/lutcode path currently shellsheygenunder any provider fallback? If a future variant does, please route it throughreportHeygenFailureat that point so the classifier applies uniformly. resolve --doctorexit code contract: is any tooling (CI, orchestrators, agent playbooks) already scripting on$?from a prior--doctor-shaped command in this repo? If so, we should decide whether the new contract is0 = ffmpeg present(weak) or0 = all-strictly-required present(strong) before merge. Best to write the contract intoSKILL.mdexplicitly.
What I didn't verify
- Real heygen CLI stderr text for domain "not found" errors (
voice not foundetc.) — asserted from CLI conventions + code path analysis, not from an actual failing invocation. If Miguel has a repro that shows heygen never emitsnot foundon non-installation paths, the blocker downgrades to a concern about defense-in-depth. - HF-runtime-interop mechanics (how the resolver interacts with runtime capability discovery) — deferring to Via / Miga per the lane split.
X-HeyGen-Client-Sourceheader handling onheygen auth status/heygen --versionprobes (assumed the CLI doesn't require it for its own metadata subcommands).
Otherwise clean execution — the SKILL reframe reads well, the file/module split (heygen-cli.mjs as the shared classifier + fix-strings source) is the right cut, and the --doctor printout is genuinely useful when the classifier is right.
— Review by Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
R1 — hyperframes #2065 at 2c9425ce
Landing after Rames D Jusso's review at 22:45Z. My lens on this one is classifier mechanical correctness + --doctor exit-code contract; Rames covered telemetry, cross-PR consistency, doc/code divergence, and the auth-timeout classifier gap. Cleanest way to post is: concur with his blocker on the "not found" misclassification, then add three unique observations he didn't cover, then note lens overlaps.
Concurrence with Rames D Jusso's blocker
🔴 heygen-cli.mjs:27-31 — lower.includes("not found") reintroduces the exact dead-end this PR is designed to eliminate. Rames named the definitive case (heygen voice speech create --voice-id <invalid> → "voice not found" → classifier says install-heygen → user installs the CLI they already have → same failure). Confirming his read against the diff:
voice-provider.mjs:14catches errors fromheygen ...invocations and now routes them throughreportHeygenFailure, which callsclassifyHeygenError.classifyHeygenErroratheygen-cli.mjs:27-31(line numbers per the diff hunk starting at :20):
if (err?.code === "ENOENT" ||
lower.includes("command not found") ||
lower.includes("not found"))
return HEYGEN_NOT_FOUND_MESSAGE;The err.code === "ENOENT" is the definitive signal for "binary not spawnable." "command not found" matches shell wrappers. Bare "not found" catches any domain-level lookup that includes that substring — voice not found, asset not found in catalog, avatar not found, pending job not found. All of those come from a heygen CLI that just successfully ran, so misclassifying them as "install heygen" is the failure mode this PR set out to prevent.
Recommended fix matches Rames's: drop the bare "not found" clause; keep the ENOENT and shell-"command not found" branches. Add a negative test:
test("does NOT classify domain-level 'not found' as CLI-not-installed", () => {
const msg = classifyHeygenError({ stderr: "voice not found" });
assert.notEqual(msg, HEYGEN_NOT_FOUND_MESSAGE);
});Related to my [[verify-classifier-empirical-premise]] discipline — the substring's empirical premise ("any 'not found' means the binary is missing") is not true for the message surface of a CLI that has installed-and-authenticated failure modes. Same shape as my #2044 miss with pix_fmt; the classifier here has to be built over the ENOENT signal, not over a substring the successful path can also emit.
Additional observations (not covered by Rames)
O1 — heygen --version without a semver token silently passes. resolve.mjs runDoctor:
} else { // heygen on PATH but no semver in --version output
checks.push({ name: "heygen version", ok: true, detail: "heygen version did not include semver", fix: "" });If heygen --version exits 0 but produces text without an X.Y.Z token (weird build, dev version, stripped release), the version check is marked ok. A stricter reading is "unable to verify" and surfaces as a warning. Optimistic-fails-open is a defensible choice, but naming the branch. Not blocker; noting for the checklist.
O2 — node version check is tautologically ok. resolve.mjs: checks.push({ name: "node version", ok: true, detail: process.version, fix: "" }). Since we're running in node, this is true by definition — it prints ✓ v20.10.0 for a check that has never had the ability to fail. If the intent is to gate on node >= X, it wants versionLessThan(process.versions.node, MIN_NODE). If it's purely informational readout, current shape is fine but the ✓ glyph implies actual verification.
O3 — Reporting-path asymmetry. reportHeygenFailure routes actionable messages to bare stderr (console.error(message)) but non-actionable through a prefixed context line (console.error(\media-use: \`${context}\` failed: ${message}`)). That's the correct asymmetry — actionable diagnostics are their own sentences and don't need a wrapper — but it means grep-based log parsing that expects the media-use:` prefix on every heygen failure will see gaps on the actionable path. If any observability tooling downstream keys on that prefix, worth knowing. Not a change request; noting for the record.
Lens overlap with Rames (concurring)
lower.includes("401")is too permissive (Rames's second Concern). Agreed — request IDs (req-401abc), URLs, retry-after headers all trip it.\b401\borHTTP 401/status: 401is the fix.emailFromAuthStatusregex extracts any email-shaped substring from prose (Rames's fourth Concern). Agreed on both the bug and the recommended fix (heygen auth status --json+ take the JSON branch only, drop the human-format bet). My draft had this as a passing observation; Rames's example —"Session expired. Contact support@heygen.ai to re-auth."returningsupport@heygen.aias the authenticated email — is definitive.- ffmpeg vs ffprobe strict-requirement doc/code divergence (Rames's first Concern). Missed this in my initial read. Doc says both required; code gates only on ffmpeg. Pick one before merge.
- No telemetry on
--doctoror its per-check outcome (Rames's fifth Concern). Sibling of my [[telemetry-gap-on-parallel-branch]] discipline — the moment a new observability affordance ships alongside existing catalog/voice telemetry paths, the new branch reliably skips instrumentation. Fix is cheap now, migration later.
Stack note
This PR's own diff is the single onboarding commit; the rest of the branch belongs to #2041. Merge #2041 first per your PR body — #2065 sits at base=feat/media-use-color-grading.
Verified
- Subprocess safety.
runCommandusesspawnSync(bin, argv, { encoding: "utf8", timeout: 15000 })— argv array, no shell, 15s timeout. --doctorexit-code contract mechanically matches PR body ("Exit 0 unless ffmpeg missing") — though Rames flagged that this contradicts the SKILL.md diff calling both strictly-required.- stdout stays JSON on both
--doctorand the failure-diagnostic paths —reportHeygenFailurewrites exclusively toconsole.error. - Classifier tests cover the 4 golden paths (ENOENT, "not logged in", old semver, passthrough) — coverage on the "not found" negative case is exactly what's missing per the blocker above.
R1 by Via
…tract, telemetry - Blocker: classifyHeygenError no longer treats a bare "not found" as CLI-missing (a stale voiceId → "voice not found" was sending users to reinstall a working CLI); keep only ENOENT + "command not found". Regression test added. - 401 now matches \b401\b, not any "401" substring (request IDs no longer misread). - --doctor: top-level ok requires ffmpeg AND ffprobe (matches SKILL.md); emits media_use_doctor_run telemetry; auth status queried with --json + JSON-only parse; auth timeout softened (network issue, not a false "unauthenticated"); node version gated on >= 18; version-without-semver labeled, not silently green. - Nits: install cmd uses && ; dropped the runResolveStatus alias. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
2c9425c to
3d85aa6
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
R2 — hyperframes #2065 at 3d85aa67
🟢 Cleared. Blocker + all substantive observations addressed. Verified per-finding at the R2 commit.
Blocker (concurring with Rames) — FIXED
"not found" broad match ✅ dropped. heygen-cli.mjs:30:
if (err?.code === "ENOENT" || lower.includes("command not found")) {The bare "not found" clause is gone. Comment at :25-29 names the exact failure mode we flagged (stale voiceId → "voice not found" → reinstall dead-end loop). Two regression tests pin it:
heygen-cli.test.mjs:44—"does not misclassify a resource 'not found' error as a missing CLI"sends"Error: voice not found (id: stale-123)"and asserts it is NOT the install message.heygen-cli.test.mjs:57—"classifies a shell 'command not found' as a missing CLI"sends"bash: heygen: command not found"and asserts it IS the install message.
Positive-and-negative both covered; regression can't reintroduce without a test change.
Other verified fixes
\b401\bregex ✅ —:39:/\b401\b/.test(lower). Test at:23-31covers"HTTP 401 Unauthorized"→ auth (positive) AND"upload failed (request req-401abc)"→ NOT auth (negative). Same positive-and-negative pin as the not-found case.ffmpeg AND ffprobestrict ✅ —resolve.mjs:848:return { ok: !!ffmpeg?.ok && !!ffprobe?.ok, checks }. Comment at:843names SKILL.md as the source of truth. Doctor test atresolve.test.mjs:333reflects the AND semantics.heygen auth status --json✅ —:748:runCommand("heygen", ["auth", "status", "--json"]). Comment at:883: "JSON only... No prose regex fallback" — the human-format bet onemailFromAuthStatusis retired.- Auth timeout softener ✅ —
:752:const timedOut = authProbe.error?.code === "ETIMEDOUT" || authProbe.signal != null;— distinct classifier for probe timeout vs authoritative unauth. Rames's "wrong actionable when network stalls" concern closed. media_use_doctor_runtelemetry ✅ —:124:await track("media_use_doctor_run", { ... });. Rames's observability gap closed.- Node ≥ 18 gate ✅ (my O2 → real check now) —
:37MIN_NODE_VERSION = "18.0.0";:835nodeOk = !versionLessThan(process.versions.node, MIN_NODE_VERSION); fix lineupgrade Node to >= v18.0.0.node versionis no longer tautologically ok. - Version-without-semver labeled
⚠️ mitigated-accepting (my O1) —:809-814: check staysok: truebutdetailnow reads"heygen present; version unverifiable (no semver in --version output)". Not a strict-verify (still fails-open), but the human output no longer implies a real version comparison happened. Acceptable tradeoff for a dev/stripped-build edge case; per my [[r2-verdict-mitigation-vs-full-resolution]] discipline this is honest-mitigation rather than full-resolution. - Install cmd
&&chain ✅ —heygen-cli.mjs:3:"curl -fsSL https://static.heygen.ai/cli/install.sh | bash && heygen auth login --key <key>"— real cut-and-pasteable shell chain (was ambiguous... | bash then heygen ...). runResolveStatusalias dropped ✅ —resolve.test.mjsusesspawnResolvedirectly; the pass-through wrapper is gone.
Not addressed (accepted)
My O3 (reporting-path asymmetry — actionable messages skip the media-use: prefix that non-actionable failures carry) wasn't touched. Confirming that's intentional: actionable diagnostics stand alone as their own sentence; contextual failures get the wrapper. The asymmetry is by design, not a bug. Fine to leave.
R2 by Via — blocker resolved, all substantive observations resolved or mitigated-accepting.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Round 2 review at 3d85aa6.
R1 blocker is resolved — bare "not found" substring gone from the classifier, and the regression test exercises the exact tweeted case (voice not found embedded next to the heygen … command line). Every 🟠 concern is addressed cleanly; two 🟡 nits landed; the Q on --doctor's exit-code contract is only partly closed (code + SKILL.md now agree, but no explicit contract section was added). Nothing new that blocks. CI green on all required checks; mergeable = MERGEABLE, mergeStateStatus = UNSTABLE (Graphite/mergeability_check pending only).
Item-by-item verification
R1 🔴 Blocker
skills/media-use/scripts/lib/heygen-cli.mjs:27-31—lower.includes("not found")will misclassify domain "not found" errors as CLI-not-installed, reintroducing the exact dead-end this PR is designed to eliminate. […] Fix: drop the bare"not found"clause; keeperr?.code === "ENOENT"(system-level: binary not spawnable) andlower.includes("command not found")(shell wrapper's own not-found line). Add a defensive test likeclassifyHeygenError({ stderr: "voice not found" })must NOT returnHEYGEN_NOT_FOUND_MESSAGE— otherwise the next refactor slides back in silently.
→ RESOLVED at skills/media-use/scripts/lib/heygen-cli.mjs:30 (commit 3d85aa6). Gate is now exactly the requested pair: if (err?.code === "ENOENT" || lower.includes("command not found")). Regression tests at heygen-cli.test.mjs:44-55 and :57-61:
- :44-55 — stderr
Error: voice not found (id: stale-123)PLUS amessage: "Command failed: heygen voice speech create …"(the toughest case, sinceclassifyHeygenErrorconcatenates.messageinto the searched text) assertsnotEqual(HEYGEN_NOT_FOUND_MESSAGE). This is the exact tweeted case and it now passes through as detail. - :57-61 — shell wrapper's
bash: heygen: command not foundstill classifies asHEYGEN_NOT_FOUND_MESSAGE, confirming the narrow retained case works.
Tests pass locally (7/7). No stale includes("not found") anywhere else in skills/media-use/.
R1 🟠 Concerns
skills/media-use/scripts/resolve.mjs:814— top-levelokisffmpegonly, butSKILL.md(this PR, line 178) claimsffmpeg/ffprobeare strictly-required. […] Pick one: either widen the gate toffmpeg && ffprobe(matches SKILL.md), or narrow the SKILL.md line to "ffmpegis strictly required;ffprobeis required for probe operations" (matches code).
→ RESOLVED at resolve.mjs:846-848: return { ok: !!ffmpeg?.ok && !!ffprobe?.ok, checks };. SKILL.md line 358 still declares "ffmpeg/ffprobe are strictly required" — two sources of truth now agree. The test at resolve.test.mjs:333-364 was rewritten to assert this: strictOk = ffmpeg.ok && ffprobe.ok; both parsed.ok and process exit code must equal strictOk. That test would fail loudly if the gate ever regresses to ffmpeg-only.
skills/media-use/scripts/lib/heygen-cli.mjs:36—lower.includes("401")matches any freeform text containing the literal substring "401". Request IDs (req-401abc), URLs, retry-after headers […] all trip this. Tighten to a word/whitespace boundary or a status-code shape.
→ RESOLVED at heygen-cli.mjs:39: /\b401\b/.test(lower). Test at heygen-cli.test.mjs:23-34 covers both directions — HTTP 401 Unauthorized classifies as auth failure, upload failed (request req-401abc) does not.
skills/media-use/scripts/resolve.mjs:849-862—emailFromAuthStatusregex captures the first@-shaped token in any prose. […] Suggest gating on a preamble […] or — better — passing--jsonexplicitly toheygen auth statusand taking the JSON path only.
→ RESOLVED at resolve.mjs:748 (invocation now heygen auth status --json) and resolve.mjs:882-894 (parser is JSON-only — early-returns null when the body isn't {…}; no prose fallback). The "Session expired. Contact support@heygen.ai" failure mode is no longer reachable — that body wouldn't start with {, emailFromAuthStatus returns null, authProbe.status === 0 gate still fails, and --doctor prints the correct unauthenticated fix.
No telemetry on
--doctorinvocation or per-check outcome.
→ RESOLVED at resolve.mjs:124-128: await track("media_use_doctor_run", { ok, checks_failed, failed: […names] }). Awaited so a short-lived doctor run flushes before exit. Emits on both success and failure paths (the failed array is empty on the green path but the event still fires), so the "how often is --doctor run" signal answers even when everyone's environment is healthy — no observability-on-failure-only gap.
skills/media-use/scripts/resolve.mjs:765,781—heygen auth statusruns with no--json, no timeout distinction between slow-network and unauth. […] Consider a distinct classifier for "probe timeout / spawn error" vs "authoritative unauth" […] or at minimum a— possible network issue if slowsoftener whenheygen --versionsucceeded butauth statustimed out.
→ RESOLVED at resolve.mjs:747-763. heygenAuthCheck now inspects authProbe.error?.code === "ETIMEDOUT" || authProbe.signal != null and, when true, returns detail heygen auth status timed out — possible network issue, not proof of sign-out and fix check network, then re-run --doctor — no wrong "re-login" prompt. Two small caveats worth noting but not blocking:
- Node's
spawnSyncon timeout setssignal='SIGTERM'(kill signal), noterror.code='ETIMEDOUT'— so thesignal != nulldisjunct is doing the real work; theETIMEDOUThalf is defensive-only. That's fine, just calling out that if you ever narrow thesignalcheck toSIGTERMspecifically, theETIMEDOUThalf won't be a safety net. signal != nullalso fires on any signal-terminated exit (SIGINT/SIGSEGV/etc.), which will fall under the same "possible network issue" copy. Not a real bug — those cases would also not be authoritative sign-outs — but the copy leans network-flavored.
R1 🟡 Nits
heygen-cli.mjs:3— double-space +then+ double-space. Prefer… | bash && heygen auth login --key <key>(real shell chain, cut-and-pasteable) or split into twofixfields.
→ RESOLVED at heygen-cli.mjs:3 — now curl -fsSL https://static.heygen.ai/cli/install.sh | bash && heygen auth login --key <key>. Cut-and-pasteable.
resolve.test.mjs:121-123—runResolveStatusis a straight alias forspawnResolvewith no added behavior; either drop it or inline.
→ RESOLVED — alias gone from resolve.test.mjs; only runResolve (execFileSync) and spawnResolve (spawnSync) remain, each with a distinct role.
resolve.mjs:765,781— passing["auth", "status", "--json"]explicitly mirrors the JSON-parse branch you already have.
→ RESOLVED at resolve.mjs:748 — invocation is heygen auth status --json; the parser is JSON-only. Same fix that closed the auth-regex concern.
R1 Questions
Cross-PR with #2041: the color-grading types (
grade,lut) are documented as local-only in the SKILL.md diff. Confirming: nograde/lutcode path currently shellsheygenunder any provider fallback?
→ CONFIRMED by code path at resolve.mjs:302-304: if (type === "grade" || type === "lut") return resolveColor(...), which routes exclusively through resolveGrade / resolveLut → matchColorLook / paramsFromIntent / freezeLibraryLut — none of which shell heygen. The reportHeygenFailure callers are only in heygen-search.mjs (bgm/sfx/image/icon catalog) and voice-provider.mjs. Grade/lut cleanly bypass the classifier today; if a future variant adds a heygen path, please route it through reportHeygenFailure so the classifier applies uniformly.
resolve --doctorexit code contract: is any tooling already scripting on$?from a prior--doctor-shaped command in this repo? If so, we should decide whether the new contract is0 = ffmpeg present(weak) or0 = all-strictly-required present(strong) before merge. Best to write the contract intoSKILL.mdexplicitly.
→ PARTIAL. Code side is settled — the contract is strong (exit 0 iff ffmpeg AND ffprobe present), enforced by the test at resolve.test.mjs:333-364 that asserts status === (strictOk ? 0 : 1). SKILL.md line 358 declares ffmpeg+ffprobe strictly-required, which implies the contract. What's missing is a plain sentence in the --doctor section (around SKILL.md:17-21 or in the CLI table at :136) that says out loud "exits 0 iff all strictly-required deps present; non-zero for any missing strict dep or unauthenticated heygen? etc." — that would remove the last ambiguity for anyone scripting on $?. Not blocking; worth a follow-up commit.
R2-NEW findings
- 🟡
resolve.mjs:835—versionLessThan(process.versions.node, MIN_NODE_VERSION)for the node-version check silently green-lights any node build whoseprocess.versions.nodedoesn't match^v?(\d+)\.(\d+)\.(\d+)$.versionPartsreturnsnullon non-plain-semver →versionLessThanreturnsfalse→nodeOk=true. For mainline node this is fine (process.versions.nodeis always plainX.Y.Z), but if anyone ever runs under a pre-release/nightly build the check goes silently green rather than either failing loudly or labeling the version as unverifiable the way the heygen path does at:809-814. Consider mirroring the heygen "version unverifiable" branch for node, or at least a strict-numeric fallback. (nit; low-priority) - 🟡
resolve.mjs:882-894emailFromAuthStatustrusts anything parsable fromparsed?.data?.email || parsed?.email. No email-shape check. Ifheygen auth status --jsonever returns{ data: { email: null } }on partial-sign-in, that renders as authenticated-as-null. Also — the value goes into a user-facing print at:757-758(heygen authenticated as ${email}) without any sanitization, so a malformed CLI output could inject arbitrary text into--doctoroutput. In practiceheygen's own JSON is trusted, so this is defense-in-depth only. (nit)
What I didn't verify
- Actual behavior of
heygen auth status --jsonin the wild (return schema, error shape). Asserted from the code path; if the CLI ever wraps the email in a different key,emailFromAuthStatusreturnsnulland--doctorreports unauth. That's a safer failure than the R1 shape, but worth a live check pre-merge. - Real Node
spawnSynctimeout semantics for the auth-status probe (whethererror.code === "ETIMEDOUT"ever gets set on this platform / node version). The|| signal != nulldisjunct catches the SIGTERM case regardless, so the classification is safe either way. - Whether the removed
runResolveStatusalias had any downstream in-repo caller (grep inskills/shows none; couldn't grep the whole monorepo from this worktree).
Clean R2. The blocker fix is precisely-shaped and pinned by the exact regression test I asked for — nice work. Only the SKILL.md exit-code sentence is left as a genuine follow-up; everything else is done.
— Review by Rames D Jusso
…tract, telemetry - Blocker: classifyHeygenError no longer treats a bare "not found" as CLI-missing (a stale voiceId → "voice not found" was sending users to reinstall a working CLI); keep only ENOENT + "command not found". Regression test added. - 401 now matches \b401\b, not any "401" substring (request IDs no longer misread). - --doctor: top-level ok requires ffmpeg AND ffprobe (matches SKILL.md); emits media_use_doctor_run telemetry; auth status queried with --json + JSON-only parse; auth timeout softened (network issue, not a false "unauthenticated"); node version gated on >= 18; version-without-semver labeled, not silently green. - Nits: install cmd uses && ; dropped the runResolveStatus alias. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
3d85aa6 to
abbfaa7
Compare
…tract, telemetry - Blocker: classifyHeygenError no longer treats a bare "not found" as CLI-missing (a stale voiceId → "voice not found" was sending users to reinstall a working CLI); keep only ENOENT + "command not found". Regression test added. - 401 now matches \b401\b, not any "401" substring (request IDs no longer misread). - --doctor: top-level ok requires ffmpeg AND ffprobe (matches SKILL.md); emits media_use_doctor_run telemetry; auth status queried with --json + JSON-only parse; auth timeout softened (network issue, not a false "unauthenticated"); node version gated on >= 18; version-without-semver labeled, not silently green. - Nits: install cmd uses && ; dropped the runResolveStatus alias. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
d570af8 to
4c8010b
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Round 3 review at 4c8010b.
R3 addresses three E2E-surfaced issues from testing against the live free-usage backend: OAuth-capable CLI floor (v0.1.6 → v0.3.0), install/auth guidance switched to --oauth for free credits (was --key), and the R2-added --json flag on heygen auth status dropped because v0.3.0 emits JSON by default (the flag is rejected as unknown on v0.3.0). Test constants refactor is a nice maintenance win.
Also lands the latestHeygenStable() best-effort CDN probe with a 4s timeout, silent on failure — good defense against getting stuck when the CDN is unreachable. Version-behind nudge only fires when we know a newer stable exists AND the installed version already clears the floor. Well-shaped.
CI green (all required checks + CodeQL + Render on windows-latest + Tests on windows-latest). mergeable = MERGEABLE.
Three 🟡 nits inline — all related to the interaction of the newly-bumped floor and the auth-status probe. No blockers; the change is otherwise clean R3.
On the SKILL.md exit-code contract sentence from R2 ("exits 0 iff all strictly-required deps present") — still not added. Two rounds in on the same follow-up; not gating merge on it but flagging that it's still open if anyone wants to script on $?.
| // 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. | ||
| export const HEYGEN_MIN_VERSION = "0.3.0"; |
There was a problem hiding this comment.
🟡 The bump from 0.1.6 → 0.3.0 fires HEYGEN_OUTDATED_MESSAGE on --doctor for anyone on 0.1.6-0.2.x, including users who never touch OAuth (API-key auth still works fine on those versions). The commit rationale is precise ("free usage needs OAuth needs v0.3.0"), but runDoctor doesn't distinguish OAuth-first from API-key-first users — everyone gets nudged to update. Two ways to read it: (a) team policy is "we require 0.3.0 uniformly, including for API-key users" — reasonable, but worth stating in SKILL.md since it's a stronger claim than the commit body implies; (b) the check should branch on credential type (heygenCredential()?.type === "oauth") and only require 0.3.0 for the OAuth path. Non-blocking either way.
— Review by Rames D Jusso
| }); | ||
|
|
||
| const nodeOk = !versionLessThan(process.versions.node, MIN_NODE_VERSION); | ||
| checks.push({ |
There was a problem hiding this comment.
🟡 heygenAuthCheck() runs unconditionally here, even when the version check just failed at line 843. On a v0.1.6-v0.2.x-installed CLI, heygen auth status doesn't emit JSON by default (that's the v0.3.0 behavior the R3 commit body relies on) — so emailFromAuthStatus early-returns null on the human-format body, the check reports "heygen not authenticated" for an actually authenticated older-CLI user, and the user sees TWO errors ("outdated" + "not authenticated") for one root cause. Consider gating: if (versionOk) checks.push(heygenAuthCheck()); else checks.push({name: "heygen authenticated", ok: false, detail: "skipped — update heygen first", fix: HEYGEN_UPDATE_COMMAND});. Same story applies to the else branch at line 870 where version is unverifiable, though that's a narrower repro.
— Review by Rames D Jusso
| // `heygen auth status` already emits JSON by default (only `--human` opts out | ||
| // to a table) — there is no `--json`/`--output` flag; passing one errors with | ||
| // "unknown flag". emailFromAuthStatus parses that default JSON. | ||
| const authProbe = runCommand("heygen", ["auth", "status"]); |
There was a problem hiding this comment.
🟡 The R3 change here — dropping --json because v0.3.0 emits JSON by default — introduces an implicit contract dependency between this probe and HEYGEN_MIN_VERSION. The parser (emailFromAuthStatus) is JSON-only and early-returns null on non-{-prefixed bodies (correctly). But if HEYGEN_MIN_VERSION is ever lowered below 0.3.0 (or if an older heygen slips through the version check per the finding above), this probe silently misclassifies authenticated users as unauth. A one-line comment linking the two facts here — e.g. // Relies on JSON-default output — see HEYGEN_MIN_VERSION >= 0.3.0 — would prevent someone later lowering the floor and quietly breaking auth detection.
— Review by Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
R3 — hyperframes #2065 at 4c8010b8714a9ffe1b95977c0a6d1c6ac0e8dbcc
Landing after Rames D Jusso's R3 at 21:33Z. Same head SHA, concurring on his three inline 🟡 nits with independent verification, adding my classifier-mechanic / auth-boundary lens on top of the pieces he didn't cover. Overall verdict: 🟢 non-blocking — the classifier fix I R2-verified is preserved, the new commit is a targeted well-shaped fix, and Rames's three follow-ups are correct but non-gating.
R2 blocker preservation — still verified
The classifier fix I R2-cleared survives verbatim at R3:
heygen-cli.mjs:31-34—if (err?.code === "ENOENT" || lower.includes("command not found"))is the ONLY path toHEYGEN_NOT_FOUND_MESSAGE. Bare"not found"clause remains gone.- The comment at
:26-30still enshrines the exact failure mode (stale voiceId →"voice not found"embedding aheygen ...command line). - Regression tests at
heygen-cli.test.mjsstill cover the tweeted case (embeddedvoice not found→ NOT install message) AND the shell miss (command not found→ IS install message).
The R3 commit does not touch classifyHeygenError or its tests — the R2 semantic is line-drift free.
R3 fix commit — targeted, well-documented
Miguel's single new commit (fix(media-use): require OAuth-capable heygen CLI (v0.3.0), fix auth-status probe) is three surgical fixes, each e2e-motivated:
- Version floor
0.1.6 → 0.3.0—heygen-cli.mjs:4. Rationale in comment:1-3: v0.1.x/0.2.x reject OAuth ("heygen-cli can't use OAuth yet"), and the free-usage path requires OAuth. See caveat below (Rames's finding 1) about the reach of this floor. - Install/auth command switched to
--oauth—heygen-cli.mjs:6-9. Matches EF #41448's server-side gating:X-HeyGen-Source: clion OAuth is what triggers the free-tier decision;--api-keyis a different billing path entirely. Comment at:5-6disambiguates. heygen auth statusprobe —resolve.mjs:759-778. Bogus--jsonflag dropped (v0.3.0 emits JSON by default;--jsonerrors as unknown).emailFromAuthStatus:904-916unchanged — still validates.startsWith("{")beforeJSON.parse, refuses to fall back to a prose regex. R2 timeout softener preserved.latestHeygenStable()update nudge —resolve.mjs:747-757.curl -fsSL --max-time 4,null-on-failure, wired at:809-823to append(latest v… available)when installed clears the floor but a newer stable exists. Fix field populated only when the probe succeeded AND the version is actually behind. This is my [[decorative-gate]] shape's inverse (informational nudge, not enforcing silent-drop) — both read-path and populate-path are complete, and the CDN-unreachable failure mode is a clean "no nudge shown," not a red doctor. Correct.
Concurring with Rames — all three verified independently
Rames landed R3 first with three inline 🟡 nits. I re-verified each in situ; concurring on all three:
-
Nit 1 (floor bump nudges API-key users too,
heygen-cli.mjs:4) — verified.runDoctoratresolve.mjs:809-826comparesheygenVersionagainst the floor unconditionally; it never inspectsheygenCredential()?.typefirst, so a user on v0.1.6 with an API key (which works fine on that CLI version) getsHEYGEN_OUTDATED_MESSAGEfor reasons that don't apply to them. Rames's two-option framing (state the uniform-floor policy vs. branch by credential type) captures the choice cleanly. From my auth-boundary lens: option (b) is cleaner but branches doctor into two paths; option (a) is simpler but sends OAuth-reasoning to non-OAuth users. Team call, non-blocking. If (a): worth a one-line SKILL.md note that "v0.3.0 is the required floor regardless of auth type." If (b): the branch should also short-circuitheygenAuthCheck(see Rames's finding 2). -
Nit 2 (
heygenAuthCheckruns unconditionally even when version fails,resolve.mjs:827and:838) — verified and this one is the tightest of the three.resolve.mjs:826pushes the version check withok: versionOk, then:827immediately callsheygenAuthCheck()regardless ofversionOk. On a v0.1.6 CLI, the auth probe runsheygen auth status— but v0.1.6 doesn't emit JSON by default (that's the v0.3.0 behavior the R3 commit rationale depends on), soemailFromAuthStatusearly-returnsnullon the human-format body, and the user sees BOTH "outdated" AND "not authenticated" for a single root cause. Rames's suggested gate (if (versionOk) checks.push(heygenAuthCheck()); else checks.push({ ... skipped — update heygen first ... })) is the right shape. From my classifier-mechanic lens: this is a version-and-format coupling. The auth probe's parser and its version-floor sit on either side of an implicit contract that isn't enforced in code — which is Rames's finding 3 restated at the doctor-level. Same underlying issue, two surfaces. Non-blocking because the current floor DOES require v0.3.0, so the double-error only fires for users trying to run--doctoron an outdated CLI — which is exactly the population being told to update anyway. But the UX is unnecessarily noisy on the exact path we WANT to lead someone through. -
Nit 3 (implicit contract dep between the probe and
HEYGEN_MIN_VERSION,resolve.mjs:763) — verified. The--json-drop is only safe because the floor is v0.3.0. If someone lowers the floor (say, to accept v0.2.x for a specific dev-machine path), auth detection silently regresses tonullfor older CLI users because theirauth statusemits human-format text thatemailFromAuthStatuscorrectly refuses to parse. Rames's one-line comment suggestion (// Relies on JSON-default output — see HEYGEN_MIN_VERSION >= 0.3.0) is the right defense — cheap, deterministic, catches the exact regression class. Concurring; would add that a symmetric comment onHEYGEN_MIN_VERSIONitself (// Lowering below 0.3.0 will silently break emailFromAuthStatus — see resolve.mjs:763) closes the loop from both sides.
Not repeating Rames's coverage
Rames's rubric already ships: CI signal (all required + CodeQL + Windows), mergeable: MERGEABLE, test-constants refactor pattern verified. I'm not re-scoring those.
Open follow-up (mine, R2)
Standing carry-over from R2: my O3 (reporting-path asymmetry — actionable messages skip the media-use: prefix that non-actionable failures carry) is still not touched. I'm confirming as intentional-by-design; not requesting a change.
Also carrying: my prior R3 forward-note about emailFromAuthStatus shape-coupling is now Rames-finding-3 stated more precisely (his comment names the reciprocal fact — that lowering the floor breaks the parser). Consolidating: the two-way comment pair Rames suggests is the fix I'd take.
R3 by Via — R2 blocker preservation confirmed; R3 fix commit lands clean; concurring with Rames on all three 🟡 nits; non-blocking overall.
…tract, telemetry - Blocker: classifyHeygenError no longer treats a bare "not found" as CLI-missing (a stale voiceId → "voice not found" was sending users to reinstall a working CLI); keep only ENOENT + "command not found". Regression test added. - 401 now matches \b401\b, not any "401" substring (request IDs no longer misread). - --doctor: top-level ok requires ffmpeg AND ffprobe (matches SKILL.md); emits media_use_doctor_run telemetry; auth status queried with --json + JSON-only parse; auth timeout softened (network issue, not a false "unauthenticated"); node version gated on >= 18; version-without-semver labeled, not silently green. - Nits: install cmd uses && ; dropped the runResolveStatus alias. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
…, floor policy
- --doctor skips the auth check when the version check fails (below v0.3.0): an
old CLI's auth probe fails for the same root cause, so users no longer see two
errors ("outdated" + "not authenticated") — one root cause, one fix.
- Comment links the auth-status probe's JSON-default assumption to
HEYGEN_MIN_VERSION >= 0.3.0 so the floor isn't silently lowered later.
- SKILL.md states the uniform v0.3.0 requirement (nudged even for API-key use).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
4c8010b to
d8a8724
Compare
…tract, telemetry - Blocker: classifyHeygenError no longer treats a bare "not found" as CLI-missing (a stale voiceId → "voice not found" was sending users to reinstall a working CLI); keep only ENOENT + "command not found". Regression test added. - 401 now matches \b401\b, not any "401" substring (request IDs no longer misread). - --doctor: top-level ok requires ffmpeg AND ffprobe (matches SKILL.md); emits media_use_doctor_run telemetry; auth status queried with --json + JSON-only parse; auth timeout softened (network issue, not a false "unauthenticated"); node version gated on >= 18; version-without-semver labeled, not silently green. - Nits: install cmd uses && ; dropped the runResolveStatus alias. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
…, floor policy
- --doctor skips the auth check when the version check fails (below v0.3.0): an
old CLI's auth probe fails for the same root cause, so users no longer see two
errors ("outdated" + "not authenticated") — one root cause, one fix.
- Comment links the auth-status probe's JSON-default assumption to
HEYGEN_MIN_VERSION >= 0.3.0 so the floor isn't silently lowered later.
- SKILL.md states the uniform v0.3.0 requirement (nudged even for API-key use).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
bb8aeaf to
cddf270
Compare
… --doctor, free-usage framing media-use resolves bgm/sfx/image/icon (catalog), voice (TTS), and avatar video through the heygen CLI — the free-usage path. Agents hit a dead end when it's missing/unauthed. This guides them to install it fast, at the point of need. - Centralized actionable diagnostics (lib/heygen-cli.mjs): every heygen-backed resolve, on failure, prints the exact fix on stderr — not-installed (curl install one-liner), not-authenticated (heygen auth login), outdated (heygen update). Routed through heygen-search + voice-provider. stdout stays clean JSON. - resolve --doctor preflight (human + --json): checks heygen present/version/ auth, ffmpeg, ffprobe, node, a fix per gap. Exit 0 unless ffmpeg missing. - SKILL reframe: install-first callout; heygen as the free-usage gateway for bgm/image/voice/avatar-video; removed the false "degrades gracefully" claim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
…tract, telemetry - Blocker: classifyHeygenError no longer treats a bare "not found" as CLI-missing (a stale voiceId → "voice not found" was sending users to reinstall a working CLI); keep only ENOENT + "command not found". Regression test added. - 401 now matches \b401\b, not any "401" substring (request IDs no longer misread). - --doctor: top-level ok requires ffmpeg AND ffprobe (matches SKILL.md); emits media_use_doctor_run telemetry; auth status queried with --json + JSON-only parse; auth timeout softened (network issue, not a false "unauthenticated"); node version gated on >= 18; version-without-semver labeled, not silently green. - Nits: install cmd uses && ; dropped the runResolveStatus alias. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
…tatus probe
E2E against the live free-usage backend surfaced three issues:
- HEYGEN_MIN_VERSION was 0.1.6, but that CLI can't use OAuth ("heygen-cli can't
use OAuth yet") — free usage needs >= v0.3.0. Bumped the floor; --doctor now
also nudges `heygen update` when a newer stable exists (always-latest).
- Onboarding pointed at `heygen auth login --key` (API credits / billing); the
free path is `--oauth` (subscription/free credits). Fixed install + auth
guidance and SKILL.md accordingly.
- `heygen auth status --json` is an unknown flag on v0.3.0 (JSON is the default
output) — the added --json broke auth detection. Dropped it; verified
--doctor reports authenticated on a real free (OAuth) account.
Tests assert against the exported message constants instead of brittle literals.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
…, floor policy
- --doctor skips the auth check when the version check fails (below v0.3.0): an
old CLI's auth probe fails for the same root cause, so users no longer see two
errors ("outdated" + "not authenticated") — one root cause, one fix.
- Comment links the auth-status probe's JSON-default assumption to
HEYGEN_MIN_VERSION >= 0.3.0 so the floor isn't silently lowered later.
- SKILL.md states the uniform v0.3.0 requirement (nudged even for API-key use).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
The 'heygen on PATH' and 'heygen version' checks both rendered their
detail as `heygen v0.3.0`, so --doctor printed two byte-identical green
lines. Make the PATH row report presence ("heygen found on PATH") and let
the version row own the version string — one row per fact, no duplicate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H5k87mPZ4d6yiFwcWSb8Vv
cddf270 to
ad9f8b2
Compare

Summary
Fast, actionable HeyGen CLI onboarding for agents, and reframes media-use around the free-usage gateway. Turns dead-end HeyGen errors into concrete install/auth/update steps, adds
resolve --doctorto preflight dependencies, and requires an OAuth-capable CLI.What / why
heygen-cli.mjsclassifier: maps failures toHEYGEN_NOT_FOUND/NOT_AUTHENTICATED/OUTDATEDwith fix commands. Keyed onENOENT/command not found(not a bare "not found", which a stale voiceId would trip);\b401\bnot a substring.resolve --doctor: checks heygen (on PATH / version / authenticated), ffmpeg, ffprobe, node — emitsmedia_use_doctor_runtelemetry; nudgesheygen updatewhen a newer stable exists.heygen auth login --oauth(free subscription credits), not--api-key(billing).auth statusprobed without--json(v0.3.0 emits JSON by default;--jsonerrors) — fixes false "not authenticated".Tests
--doctorverified against a real v0.3.0 + OAuth account.Stack (merge bottom→top): #2027 → #2065 → #2113