Repository navigation
fix(media-use): use a host app's music tools; fail loudly without the heygen CLI - #5154
miguel-heygen wants to merge 14 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
… keep the sfx test offline
…LI; sync the skills manifest
terencecho
left a comment
There was a problem hiding this comment.
Review at 1355c7c6dd09433d3aa3199efeba13abcb349b28 — requesting changes on the host-audio workflow contract. The new CLI diagnostics pass their focused tests; the concerns below are with the instructions that consume host audio.
-
Host-created SFX are replaced or dropped by the prescribed
fetch-sfxpass. The new host-first direction inskills/product-launch-video/SKILL.md:10(alsopr-to-videoandfaceless-explainer) has no sound-effect handoff corresponding to the new BGM handoff at :131. Step 5 still runsaudio.mjs fetch-sfx(:175), which derives names from storyboard cues, resolves them against HeyGen/bundled sounds, then rewrites the entireaudio_meta.jsonfrom its neutral sidecar (scripts/audio.mjs:200-223). I reproduced a host-providedassets/sfx/custom-whoosh.mp3entry already inaudio_meta.jsonbeing replaced with the bundledassets/sfx/whoosh.mp3by that pass; a custom cue without a library match is skipped. Assembly reads onlyaudio_meta.json.sfx, so the host-generated audio never reaches the film. Please give host SFX an explicit, stable path into the final metadata after the rewriting pass (or make the pass preserve/adopt them), and cover that flow. -
The no-script
music: nonepath still tells the agent to skip SFX. The new Step 3.1 text atskills/product-launch-video/SKILL.md:131says that when a host supplies BGM, usemusic: none, and even withoutSCRIPT.mdrunfetch-sfxfor any frame cues. Four lines later (:135), the same combination is declared “fully silent” with “no SFX”; Step 5 (:171) says to skip both duration sync andfetch-sfxwhen silent. In a non-narrated host-music video whose storyboard hassfx: whoosh, following that Step 5 gate skips the only pass that writes SFX entries; the later manual BGM entry does not restore those sounds. The copied workflow text inpr-to-videoandfaceless-explainer, plus theirreferences/story-design.mdsilent instructions, need the same reconciliation. Theaudio.mjsgenerate shortcut for this marker (:144-159) makes the contradictory interpretation particularly likely.
Nonblocking follow-ups: skills/media-use/SKILL.md:12 and references/setup-providers.md:20-24 still require HeyGen CLI installation/sign-in on first run even if the only requested audio comes from a host app; condition setup on using HeyGen. An older CLI emitting unknown flag: --headers without a version is still classified as other and returns a generic miss (base and head behave identically), so the new outdated-CLI remedy covers only errors that include a parseable version. Please add concept-level positive-pin tests for the changed agent-facing host audio and no-script/SFX instructions; the new tests only pin CLI error messages, not this workflow.
Focused local tests: resolve.test.mjs 61/61 and heygen-cli.test.mjs 23/23. Required CI checks passed on this head. No approval or merge performed.
— tai
…needs no sound cues
…meta.json, survives a re-run
…car instead of crashing
|
Thanks, both points were real. Fixed at 5a45911:
Non-blocking items, also done:
|
terencecho
left a comment
There was a problem hiding this comment.
Review at 5a45911163080e1774e54b2d00932872433109ca — requesting changes. The two issues from my previous review are fixed: host BGM/SFX already present before a pass survive generation and fetch-sfx, and a no-script project with SFX/host audio no longer takes the fully-silent path. Focused tests and real assembly exercises passed.
Preserve host audio added while a background audio pass is running. In skills/product-launch-video/scripts/audio.mjs:153,198-201 (and the corresponding narrated adapters), runGenerate reads host entries before running the synchronous audio engine, then writes the rebuilt metadata using that old snapshot. runFetchSfx does the same at :251,259-262. The SKILL.md launches generation in the background and says every pass keeps host entries. I reproduced both paths at this head: start the engine, add valid host BGM and SFX entries/files to audio_meta.json while it runs, then let it finish. Both commands exit 0 yet leave { "bgm": null, "sfx": [] } without warning. Re-read/merge host entries at the final write (with an appropriate conflict rule), or otherwise establish and enforce sequencing that prevents a host write during the pass. A pre-pass-only snapshot is not sufficient for the documented guarantee.
Regenerate the skills manifest after the final skill edits. The Skills: manifest in sync check fails at this head: faceless-explainer is committed as 36f434caf058f16e but computes 65d08c8732942516; product-launch-video is committed as 92a85e48bb7d95c2 but computes c767ff7f62a78a54. The base passes the check, and each discrepancy is explained by a one-line test edit made after the manifest was generated. Please rerun bun run --cwd packages/cli gen:skills-manifest and commit the result. Required checks pass; this separate check is still red.
The host-audio race is the substantive remaining finding. No separate objection to the CLI compatibility behavior or mixed-source SFX cue policy was verified.
— tai
|
Thanks, both real. Fixed at b2f1098:
Also in this head: a flaky telemetry test in |
terencecho
left a comment
There was a problem hiding this comment.
Review at b2f10984ed5bbbb80f33c2d4ff78b02930781c9b — approved.
The two previous host-audio findings are resolved. Both narrated generate and fetch-sfx re-read host metadata after the engine pass, so entries added during an in-flight pass survive; the earlier deterministic race reproduction now retains both host BGM and SFX. The no-script/SFX and sequential assembly paths also work, and the skills manifest now passes its generator check. Focused audio/helper tests passed 70/70 and the media resolver suite 61/61 (one optional core-conformance assertion self-skipped in a disposable checkout lacking tsx). The skill copies remain in sync.
Nonblocking follow-up: in the fully silent branch (music: none, no script or cues), if audio_meta.json names a host file that has disappeared, the initial host scan suppresses its warning; scored is false and the branch deletes metadata while printing only "project marked silent." The previous head warned that the host file was missing. Valid host files are preserved; please restore the missing-file warning before that early cleanup so the operator does not mistake a broken host path for an intentional silent film.
— tai
Edit accuracy: accurate 2059 (base branch 2059), smooth 1460 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
What changed
Agents running inside a host app that has its own music and sound-effect tools were picking
media-use resolve --type bgminstead, because the skill called itself "the single skill for every media need". Without theheygenCLI that path fails, and its JSON answer read like "no music":{"ok":false,"error":"no provider could resolve bgm: ..."}. The real reason only went to stderr.Two ends:
skills/media-use/SKILL.mdno longer calls itself the single skill for every media need. Its description and body now say: when the host app provides its own music or sound-effect tools, use those;resolve --type bgm|sfxneeds the heygen CLI.bgmandsfxrows name the CLI they search through.references/setup-providers.mdand the catalog's compressed copies inCLAUDE.md,README.mdanddocs/prompting/overview.mdxcarry the same clause.resolvefails loudly. When a resolve misses after the heygen CLI was missing or too old, the error is one line naming what is missing and the alternative, and the JSON carries a code and the fix:{"ok":false,"code":"heygen_cli_missing","fix":"Install the CLI from https://developers.heygen.com/cli, then run: heygen auth login --oauth","error":"bgm needs the heygen CLI, which is not installed: use your host app's own music tool if it has one, or install the CLI."}An outdated CLI gives
heygen_cli_outdatedwithheygen update. Only music and sound effects get this message, where a host app's own tool is the known alternative; every other type keeps today's generic miss. The remediation was already recorded (consumeHeygenRemediation), but only a successful resolve read it; the miss path now reads it too.sfxstill answers from its bundled library first, so this only shows when nothing bundled matches."source": "host": music asbgm, a sound as ansfxentry with its frame and length. Both audio passes (generate andfetch-sfx) keep it as long as its file exists, and a frame with a host sound gets no looked-up cue. A pass reads the host's entries again at its final write, so host audio added while the engine runs is kept too, and a host sound wins its frame over a looked-up one. The rule lives in one module,skills/media-use/audio/scripts/lib/host-audio.mjs, copied into the three workflows byscripts/generate-skill-module-copies.mjslikebgm-volume.mjs. A frame written as "1" counts as frame 1; an audio_meta.json that does not parse stops the pass instead of losing the host's entries; a host entry whose file is gone, or that has no path, is dropped with a warning saying why.sfx:cues and no host audio. A film with no narration andmusic: nonebut with sound cues or host audio is no longer treated as silent: the generate pass writes the audio_meta.json file instead of deleting it, sofetch-sfxfills it, and running generate again there keeps the soundsfetch-sfxalready found. The SKILL.md marker, its gate and eachreferences/story-design.mdsay the same.--headerswithout printing a version is now reported as outdated.What I measured
On Linux:
heygenonPATH, as CI runs them,node --testover the five affected test files reports 67 pass, 0 fail, 3 runs in a row; inside itresolve.test.mjspasses 61 of 61. Three new resolve tests (music without the CLI, a sound effect nothing bundled matches, music with an outdated CLI) fail on main's code and pass with the change. The sound-effect test serves the local media index from a local server that answers 404, so it never reaches the network.heygenMissfails the newheygen-clitest.audio.test.mjs(faceless-explainer's copy is pinned byte-identical to pr-to-video's) fail on main'saudio.mjsand pass with the change:fetch-sfxkeeps a host sound and looks up no cue for its frame;fetch-sfxkeeps a host bed; no narration +music: none+ a sound cue is not silent; generate keeps a host bed.host-audio.test.mjspins the rule, and a text test pins the host contract and the meaning of silent in all three workflows (reverting one skill's gate line fails it).--headerstest fails without its new clause.oxlint .over the whole repo exits 0.fetch-sfx, and a generate re-run keeps the looked-up sounds, in each of the two workflow test files; a string frame inhost-audio.test.mjs) fail on the previous head's code and pass now.heygenonPATH: both files pass 3 runs in a row; with a realheygenpresent they pass too. The existing "--json returns error JSON on stub provider failure" test used whateverheygenthe machine had, so it now gets a fake one that finds nothing.skills-manifest.jsonis regenerated after the last skill edit.resolve.test.mjsran the CLI with a blocking spawn, which froze the test's own local telemetry server until the child gave up on its 1.5 s telemetry timeout, so the event arrived or not by timing. It now spawns asynchronously and the polling loop is gone. It failed once in 13 runs before; after the change the file passed 13 runs in a row, and the capture now also fails if the resolve run exits non-zero.lint-skills(33 skill files), the skill mirror check,check-skill-import-boundariesandlint-skillstests (46 pass), formatting and the comment checks pass.heygenonPATH:--type bgmand--type sfx --intent "dog barking"print the line above and exit 1;--type sfx --intent "whoosh"still resolves from the bundled library.What I did NOT exercise
heygeninstalled but not signed in): it is not recorded as a remediation today, so it still reports the generic miss.local SFX index unavailable), before this message. That predates this change.CI status
The Studio timeline viewport gate is red. Its overscan regression is owned by #5151, which is open; this PR does not change the viewport implementation. Other checks are still completing.