Skip to content

fix(skills): check local render dependencies before design - #5339

Open
user-github-me wants to merge 22 commits into
heygen-com:mainfrom
user-github-me:fix/early-render-prerequisites
Open

user-github-me wants to merge 22 commits into
heygen-com:mainfrom
user-github-me:fix/early-render-prerequisites

Conversation

@user-github-me

@user-github-me user-github-me commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Local-video workflows check FFmpeg and FFprobe after project setup or resume, before planning and design, and retain a passing result for the run. Missing binaries surface their setup route early. Deck, source-port, and explicit cloud tasks keep their own prerequisites; existing project wrappers retain precedence.

The macOS FFmpeg hint offers both Homebrew and existing executable binaries configured through HYPERFRAMES_FFMPEG_PATH, HYPERFRAMES_FFPROBE_PATH, or the project bin directory. The troubleshooting guide documents the two-binary check. doctor --json exits zero and its top-level ok includes optional tools, so the workflow checks FFmpeg and FFprobe individually.

The mandatory dependency check before design remains subject to maintainer approval, as requested in review.

Validation against main 89081b91971c219949677f061284ffa491921a57:

  • Fresh frozen dependency install and full workspace build passed.
  • All 124 FFmpeg, preflight, doctor, and skills-manifest tests passed, together with five existing core-skill tests.
  • The seven-skill manifest was regenerated from a clean tracked export. Official plugin packaging and recomputing every exported skill's hash and file count agree with that manifest.
  • CLI typecheck, root lint, changed-file lint and formatting, test reachability, normal commit hooks, and both post-commit comment checks passed.
  • Both entry skills stay within their 7,168-byte budgets: 7,136 and 7,154 bytes.
  • Mint documentation validation and broken-link checks passed.

The entry skills retain the root-composition and scene-mounting contract, describe the person editing in Studio, and preserve the instruction to read plugin rules before setup or refresh. The prerequisites bullet links to the full guidance to stay within the size limit. These wording changes passed a full build, 101 skill/manifest tests, skill lint, mirror checks, clean-export manifest verification, normal hooks and both comment checks.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code and docs are correct. Two things stand between this and an approval: it needs a rebase, and a maintainer has to sign off on the workflow change.

Rebase: git merge-tree against current main shows a conflict in skills-manifest.json only. Regenerating the manifest after the merge should be all it needs.

What I checked:

  • packages/cli/src/browser/ffmpeg.ts:108 gives darwin a longer hint: brew install ffmpeg, or the download page plus HYPERFRAMES_FFMPEG_PATH / HYPERFRAMES_FFPROBE_PATH / .hyperframes/bin/. Both of those configuration routes already exist and are documented in doctor-browser.md § "Configure FFmpeg without a package manager" (#5237). The tests in ffmpeg.test.ts and preflight.test.ts pin the new text. I couldn't run them locally because of a workspace-package resolution issue in my checkout, but the CLI lanes passed in CI at this head.
  • Dropping the jq -e '.ok' gate from skills/hyperframes-cli/SKILL.md:99 is correct. In doctor.ts, top-level ok also covers Docker, whisper, Kokoro and MusicGen, so that gate fails a machine that is fine for local encoding. The new two-tool gate in doctor-browser.md:72 checks the right thing.
  • The new step appears in three places: skills/hyperframes/SKILL.md:50 (the entry skill), hyperframes-cli/SKILL.md:20 and motion-graphics/SKILL.md:96. All three point at one anchor in doctor-browser.md:29-35. They agree with each other.

For the maintainer decision:

  • Every local-video run gets one doctor --json call before design. Doctor runs all its checks, including docker info with a 5s timeout. The cost is a few seconds per run, plus about 120 words added to the entry skill.
  • motion-graphics loses the one-line brew install node ffmpeg prerequisite in favour of a link.
  • Linux keeps the bare package-manager hint while macOS gets the long one. That's reasonable, because the issue is specifically about macOS without Homebrew.

Optional: the paragraph in skills/hyperframes/SKILL.md:50 could be cut to about two sentences that point at the anchor. The rules are already in doctor-browser.md.

— Rames

@jrusso1020

Copy link
Copy Markdown
Collaborator

Thanks for this, @user-github-me. This PR now has merge conflicts with main. Could you rebase it onto the latest main? Once it's conflict-free we'll review it and get it into the merge queue.

@mintlify

mintlify Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Preview approval required

This pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit c93f3ce.

Approve preview

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for carrying this through the skills reorganisation. The code part is still right: the macOS hint now offers both Homebrew and existing binaries through HYPERFRAMES_FFMPEG_PATH / HYPERFRAMES_FFPROBE_PATH or .hyperframes/bin/, and the preflight test goes through runEnvironmentChecks(), which is what doctor runs. The new section in doctor-browser.md reads well and checks FFmpeg and FFprobe individually instead of the top-level ok.

One thing that is new since the last review. Because the main entry skill was already at its 7,168-byte budget, this version rewrites some unrelated opening lines to make room, and one of those rewrites loses meaning:

  • skills/hyperframes/SKILL.md:12 used to say "index.html is the root composition: it declares the video's variables and mounts one scene per sub-composition from compositions/". The new line drops "root composition" and "mounts", and nothing else in the entry skill says that index.html mounts the scenes. An agent reading only this file could write scenes into compositions/ and never mount them. I'd keep the original sentence and find the bytes elsewhere. One option is to shorten the new work-loop bullet at :45 to just the link, since doctor-browser.md already holds the rules.
  • Smaller: line 10 now says "Studio edits the same files" (it is the person who edits them in Studio), and the plugin-install line at :6 in both entry skills lost its "Before".

The open question from the earlier review is still for a maintainer: whether every local-video run should do a doctor --json check before design starts. CI is green at this head and it merges cleanly with main.

Verdict: COMMENT
Reasoning: The code change is correct, but the entry-skill rewording drops the root-composition and mount statement, and the mandatory pre-design check still needs a maintainer's decision.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this addresses everything from my last review. The entry skill again says that index.html is the root composition that mounts one scene per sub-composition, word for word as on main; the Studio line and the "Before setup or refresh" wording are back; and the work-loop bullet is now just the link, with the timing kept in the CLI skill and the section heading. Both entry skills stay under their byte budget, and the manifest check passes.

The one thing left is the same maintainer decision as before: whether every local-video run should do a doctor --json check before design starts. The code and docs are ready either way.

Reviewed head c93f3ce8. CI is green and it merges cleanly with main.

Verdict: COMMENT
Reasoning: All review points are addressed and CI is green; only the maintainer decision on the mandatory pre-design check remains.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants