refactor(skills): rebuild faceless-explainer + pr-to-video on the shot-sequence architecture

[WaterrrForever]

What

Rebuilds faceless-explainer and pr-to-video on product-launch-video's current authoring architecture (the time-coded shot sequence) and catches their shared engine up to its latest version, while re-narrating each skill to its own domain. The shared hyperframes-animation rule/blueprint library is untouched.

Why

Both skills were a generation behind product-launch-video on two axes at once:

• Authoring model — they still used the older effects-id "phased note" instead of a voiceover-paced, time-coded shot sequence, so frames tended to front-load then freeze (the "animated slide deck" failure).
• Shared engine — the pre-assembly frame guards + BGM loop-extend (assemble-index), dark-ground caption contrast (captions), role-aware accent + mono role (tokens), and dark-mode polarity invert / weight-clamp / icon-font filter (build-frame) had only ever landed in product-launch-video. The other two copies lagged — pr-to-video videos with BGM even had silent tails.

How

• Authoring model: visual-design directs a time-coded shot sequence; motion-language carries the move vocabulary + the tightened doctrine (power3 over bouncy) + the seek-safe core (fromTo entrances, no CSS-transition motion); added cut-catalog (within-frame seams); frame-worker + SKILL Step 4/5 moved to blueprint instantiation + the two seek-safe self-check codes.
• Engine caught up: the guards / BGM / dark-ground / accent+mono / dark-mode fixes above, all inert when a skill has no brand tokens.
• faceless-explainer: folded composition.md into visual-design (inventing-the-visual / portrait / caption geometry); kept its explainer story doctrine; grafted cue-segmented VO + candidate-blueprint-at-Step-3.
• pr-to-video: kept the ingest pipeline (fetch-pr / ingest / fetch-people-avatars) + code-vocabulary; preserved the code-beat (the code-* block is the focal, Scenes choreograph the surround) and mechanism-beat treatments, rewritten under the shot-sequence model.
• Cleanup: dropped stage-assets from both (no captured assets to stage); removed derivation/provenance references so each skill reads standalone. composition.md removed; its six internal references repointed.

Notable: the engine scripts now differ from product-launch-video's copies only in comments + the adapter function name — no functional divergence, relevant if these are centralized later.

Test plan

• bun run scripts/lint-skills.ts passes for all 19 skills; skills-manifest.json regenerated; all skill scripts node --check clean; oxfmt / commitlint pre-commit hooks green.

• Domain isolation verified (faceless-explainer has no code-beat concepts; pr-to-video wires code-vocabulary across SKILL + visual-design + frame-worker + story-design); no dangling local refs; all ../hyperframes-{animation,core,creative,media} paths resolve.

• Unit tests added/updated — n/a (skill docs + scripts; no unit-test surface for this change)

• Manual testing performed — static checks above; end-to-end render not yet run (a sample explainer + a sample PR video are the remaining manual check)

• Documentation updated — n/a (skill names + router routing unchanged, so README / CLAUDE.md / skills.mdx / /hyperframes need no update)

[miga-heygen]

Review: refactor(skills): rebuild faceless-explainer + pr-to-video on the shot-sequence architecture

SHA reviewed: bdbcc0b3c888f3b50d397f056828a527e7957af8

This is a large, well-structured PR that migrates two skills (faceless-explainer and pr-to-video) to the shot-sequence authoring model and catches their shared engine scripts up to the latest product-launch-video version. I read the full diff (~4,000 lines). Here is my assessment.

---

CI: skills-manifest.json is out of sync (blocking)

The Skills: manifest in sync check failed. CI says the committed hashes don't match what the manifest generator produces:

~ faceless-explainer (0c911913a6e9c131 → b94e52c77d04c1ea)
~ pr-to-video (4cf75c084893024e → 85a7577aee41bb82)

This means a skill file was edited after bun run scripts/lint-skills.ts last regenerated the manifest. Fix: bun run --cwd packages/cli gen:skills-manifest, commit, push. All other checks pass.

---

Engine scripts — thorough and well-guarded

The assemble-index.mjs, build-frame.mjs, captions.mjs, and tokens.mjs changes are substantial and high quality:

Highlights:

• Pre-assembly frame guards (1/2/3) — auto-repair of missing data-width/data-height, hard-fail on sub-comp <video>/<audio>, and missing class="clip" / same-track overlap detection are all excellent defensive additions. These catch common worker bugs at assembly time instead of after a wasted render.
• BGM loop-extend — ensureBgmCovers() is a pragmatic fix for the silent-tail problem, with graceful degradation when ffmpeg is absent.
• Dark-mode polarity inversion in build-frame.mjs — the invert / flipL logic for dark-ground brands landing on a light preset is well-reasoned and the Math.abs(li - lc) < 40 contrast assertion is the right fix for the old "ink must be darker than canvas" check.
• Accent selection via role diversity + palette prominence — the new pickAccent sort is more robust than the old "top interactiveBg" rule.
• Icon-font filtering, weight clamping, mono font routing, brand-adaptation note, font-file staging — all clean and address real failure modes.
• Dark-ground caption contrast in captions.mjs — the lum < 90 auto-override for word states is a good fix.
• Font path fix — ../assets/fonts → assets/fonts (root-relative) fixes the lint invalid_parent_traversal_in_asset_path and Studio 404. Good catch.
• TOCTOU race fix — replacing existsSync → readFileSync + try/catch addresses the CodeQL js/file-system-race flag.

One observation on the engine scripts:

• assemble-index.mjs and build-frame.mjs are now byte-identical across faceless-explainer and pr-to-video (the PR body notes they differ only in comments + adapter function name). The duplication is acknowledged and flagged for future centralization — fine for now, but worth tracking.

---

Skill doc rewrites — correct and consistent

The authoring model migration (effects/blueprint/composition-note → time-coded shot sequence with Scene lines) is applied consistently across both skills: SKILL.md, visual-design.md, motion-language.md, story-design.md, and frame-worker.md all move in lockstep.

• composition.md deleted from both skills — its content is folded into visual-design.md (Layout section) and inline Scene lines. Clean: the old file was a redundant layer.
• cut-catalog.md added to both skills — identical content, a within-frame seam vocabulary (zoom-through, inverse, cut-the-curve, waterfall). Well-written and fills a real gap.
• motion-language.md restructured into 3 parts (vocabulary, doctrine, seek-safe core) — the move vocabulary with backing rule ids is a major improvement over the old intent-only layer.
• ANIM_DIR → RULES_DIR rename in worker contracts — consistent and correct (workers now get the rules/ subdirectory, not the whole animation skill).
• toProductLaunchMeta → toFrameKeyedMeta — clean domain-neutral rename in audio.mjs.

Minor nit on the docs:

• Both cut-catalog.md files are byte-identical. If these are meant to stay identical across skills, consider a shared symlink or a single copy under hyperframes-animation/ to avoid drift. Not blocking.

---

Type safety & code correctness

• All new JS code avoids any-style patterns (this is plain JS, not TS, but the discipline is clean).
• Number.isFinite guards on parsed durations in ensureBgmCovers and guardFrame are correct.
• The guardFrame function correctly blanks comments + <script>/<style> bodies before scanning for <video>/<audio> and timed elements, preventing false positives from GSAP code or HTML comments — nice edge case handling.
• The regex OPEN_TAG correctly handles quoted attribute values containing > — good.

---

Deterministic rendering

No Date.now(), no unseeded Math.random(), no render-time network fetches introduced. The spawnSync calls in ensureBgmCovers are build-time (assembly), not render-time. Clean.

---

Missing test coverage

The PR body notes "Unit tests added/updated — n/a (skill docs + scripts; no unit-test surface for this change)" and "end-to-end render not yet run." The engine script changes (tokens.mjs pickAccent sort, build-frame.mjs polarity inversion, guardFrame logic, ensureBgmCovers) are the kind of logic that would benefit from unit tests, but the PR body is honest about this and the skills test CI check does pass. Not blocking, but worth considering adding regression tests for the accent-selection and polarity-inversion logic in a follow-up.

---

Verdict

LGTM with one blocking fix: regenerate skills-manifest.json and push. Once CI goes green this is ready to merge. The engine improvements are solid and the authoring-model migration is thorough and consistent.

— Miga

[jrusso1020]

Additive review on top of Miga's pass. Miga covered the engine improvements + doc rewrites consistently + flagged the manifest CI blocker. My additive lane (per the delegation): the parity-claim verification, consolidation integrity, and scope check. Three verified-positive findings + two additive flags.

Verified positive

Engine parity is real + exactly as claimed

The PR body says the engine scripts "differ from product-launch-video's copies only in comments + the adapter function name — no functional divergence." Hash-compared all 7 script files at PR head between the 3 skills:

Script	Status
build-frame.mjs	byte-identical across all 3 skills
captions.mjs	byte-identical across all 3 skills
lib/tokens.mjs	byte-identical across all 3 skills
assemble-index.mjs	fe ≡ pr; diff vs plv = 1 comment line (skill-naming)
transitions.mjs	fe ≡ pr; diff vs plv = 1 comment line
lib/dimensions.mjs	fe ≡ pr; diff vs plv = 1 comment line
audio.mjs	three-way different, but ONLY comments + toProductLaunchMeta → toFrameKeyedMeta adapter rename (function bodies unchanged)

Body claim verified: zero functional drift, all differences are skill-naming comments + the one adapter rename in audio.mjs. This is the cleanest parity claim I've seen on a skill-refactor PR — typically I'd expect one or two minor behavioral drifts to creep in across N copies; here there are none.

Consolidation integrity — composition.md content preserved

The PR body says composition.md was folded into visual-design.md. Verified:

• Old composition.md (123 lines, deleted): "Composition — visual-design judgment" with Squint test / Canvas zones / Safe margin / Primary content area / Caption band / Portrait & square sections.
• New visual-design.md (146 lines, +104/-42): contains the "Squint test" (line ~87) and "Portrait & square" (line ~92) sections at PR head. Density / canvas-zone framing absorbed.
• New cut-catalog.md (215 lines, NEW): a sibling addition (within-frame seams — Zoom-Through, Cut the Curve, Waterfall) — not a rename of composition.md. Two different references.

Composition's content is preserved in visual-design.md; cut-catalog.md is genuinely new motion-recipe content.

hyperframes-animation untouched ✓

git diff --name-only origin/main.._pr1778 | grep hyperframes-animation → no hits. Confirmed.

Additive flags

Body-vs-diff scope mismatch — hyperframes-cli changes not mentioned

The PR body's scope is "rebuild faceless-explainer + pr-to-video" plus the engine catch-up + manifest. But the diff also includes:

• skills/hyperframes-cli/SKILL.md (+6/−2)
• skills/hyperframes-cli/references/preview-render.md (+42/−0)

These look like a separate intentional feature — preview --selection --json and preview --context --json Studio-context flags + a cross-cutting docs note in SKILL.md for agent-driven editing workflows. The skills-manifest.json hyperframes-cli hash change (which initially made me think "manifest regen drift") is explained by these in-PR doc changes.

Not blocking — the changes themselves look clean — but the PR body should mention them so future archaeology + reviewers + the auto-generated changelog all see the same scope. (Per REVIEW_DISCIPLINE rule 3: PR body claims should align with the diff.)

Stale stage-assets.mjs comment in lib/assets.mjs

The PR body says "Cleanup: dropped stage-assets from both." Verified: scripts/stage-assets.mjs does NOT exist for faceless-explainer or pr-to-video at PR head (and didn't at master either — these two skills never had the standalone script). It still exists in product-launch-video and music-to-video.

However, lib/assets.mjs at line 2 still carries this comment:

// Shared by stage-assets.mjs (Step 4 close, BEFORE the frame workers run) and

For faceless-explainer + pr-to-video specifically, there is no stage-assets.mjs consumer — assemble-index.mjs is the only caller of stageAssets(). The comment is correct for product-launch-video but stale for these two skills.

Cosmetic, not blocking — the function still works because assemble-index.mjs imports + calls it correctly. Would be cleaner if the comment reflected per-skill reality OR if lib/assets.mjs was genuinely a shared sibling (centralized later, as the body itself notes).

Concur with Miga + the CI blocker

• Skills: manifest in sync failure — committed hashes don't match the regen output. bun run --cwd packages/cli gen:skills-manifest + commit. Same shape as the blocker on #1745.
• Miga's engine improvements solid, doc rewrites consistent, and suggested deduplication observations all hold up — the parity verification above is actually evidence FOR Miga's deduplication suggestion (since the engine scripts are now ~99% byte-identical, centralization is a low-risk future cleanup).

Verdict

COMMENT — external author, routing through <@U08E7PV788Z> for any stamp/merge call per protocol. CI manifest fix is the only true blocker; my two additive flags are scope-clarity + cosmetic. No remaining blockers from me.

— Jerrai

[jrusso1020]

R2 verification at 43cc44f1 — manifest blocker resolved.

• Skills: manifest in sync — now passing. The single new commit (43cc44f1 chore(skills): resync skills-manifest after format pass) regenerated the manifest hashes. CI now 32/32 green (deduped from 39 raw rollup entries).
• My two prior additive flags (non-blocking, surfaced for future polish):
  • Body-vs-diff scope: the unannounced hyperframes-cli SKILL.md + references/preview-render.md changes are still in the diff but unmentioned in the body. Cosmetic for archaeology / auto-changelog clarity; not a blocker.
  • Stale stage-assets.mjs comment in lib/assets.mjs line 2 for faceless-explainer + pr-to-video. Cosmetic only; the function works correctly.

No remaining blockers from me. Still COMMENT (external author, stamp routing through <@U08E7PV788Z>).

— Jerrai

[miga-heygen]

Re-review at 43cc44f1

The previous review at bdbcc0b3 flagged one blocking issue: skills-manifest.json was out of sync for both faceless-explainer and pr-to-video.

What changed

Single new commit: 43cc44f1 — chore(skills): resync skills-manifest after format pass

The commit updates exactly two hash values in skills-manifest.json:

• faceless-explainer: 0c911913a6e9c131 → b94e52c77d04c1ea
• pr-to-video: 4cf75c084893024e → 85a7577aee41bb82

No other files touched. No code changes, no doc edits, no new logic — purely the manifest regeneration that was requested.

CI status

Skills: manifest in sync — passing. The blocker is resolved.

All other checks are green or correctly skipping (change-detection gates). No failures. Full list of passing required checks: Format, File size check, Semantic PR title, Skills: manifest in sync, Test: skills, CodeQL (actions + python + javascript-typescript), WIP, and all regression/preview/perf/player-perf gates (correctly skipped — skills-only change).

Verdict

The blocking CI issue from the previous review is fixed. The substantive review from bdbcc0b3 (engine scripts, skill doc rewrites, type safety, deterministic rendering) still applies — no functional code was changed in the new commit. This PR is ready to merge.

— Miga

[miguel-heygen]

Verified current head 43cc44f1: the blocker follow-up commit is manifest-only (skills-manifest.json hash updates), Skills: manifest in sync is green, and there are no failed/pending check runs. Substantive review from Miga/Jerrai carries; no new code/doc delta in the blocker fix.

Verdict: APPROVE
Reasoning: The only blocking issue was the stale skills manifest, and the current head fixes exactly that with CI green/skipped as expected.