Repository navigation
fix(skills): check local render dependencies before design - #5339
user-github-me wants to merge 22 commits into
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
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:108givesdarwina longer hint:brew install ffmpeg, or the download page plusHYPERFRAMES_FFMPEG_PATH/HYPERFRAMES_FFPROBE_PATH/.hyperframes/bin/. Both of those configuration routes already exist and are documented indoctor-browser.md§ "Configure FFmpeg without a package manager" (#5237). The tests inffmpeg.test.tsandpreflight.test.tspin 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 fromskills/hyperframes-cli/SKILL.md:99is correct. Indoctor.ts, top-levelokalso covers Docker, whisper, Kokoro and MusicGen, so that gate fails a machine that is fine for local encoding. The new two-tool gate indoctor-browser.md:72checks the right thing. - The new step appears in three places:
skills/hyperframes/SKILL.md:50(the entry skill),hyperframes-cli/SKILL.md:20andmotion-graphics/SKILL.md:96. All three point at one anchor indoctor-browser.md:29-35. They agree with each other.
For the maintainer decision:
- Every local-video run gets one
doctor --jsoncall before design. Doctor runs all its checks, includingdocker infowith a 5s timeout. The cost is a few seconds per run, plus about 120 words added to the entry skill. motion-graphicsloses the one-linebrew install node ffmpegprerequisite 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
|
Thanks for this, @user-github-me. This PR now has merge conflicts with |
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
miguel-heygen
left a comment
There was a problem hiding this comment.
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:12used to say "index.htmlis the root composition: it declares the video's variables and mounts one scene per sub-composition fromcompositions/". The new line drops "root composition" and "mounts", and nothing else in the entry skill says thatindex.htmlmounts the scenes. An agent reading only this file could write scenes intocompositions/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, sincedoctor-browser.mdalready 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
left a comment
There was a problem hiding this comment.
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.
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 --jsonexits zero and its top-levelokincludes 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: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.