refactor(skills): move product-launch / pr-to-video / faceless-explainer onto the script-driven architecture

[WaterrrForever]

What

Restructure the three narrated workflow skills — product-launch-video, pr-to-video, faceless-explainer — onto the shared script-driven authoring architecture. Each now authors through the common backend (build-frame remixes a hyperframes-creative preset onto brand tokens, audio routes through the hyperframes-media engine, assemble-index builds the standalone index.html) instead of its own bespoke scripts.

4 commits, 629 files (+8322 / −87291 — the large deletion count is the removal of each skill's old per-skill scripts in favour of the shared lib):

Commit	Scope
refactor(product-launch-video)	restructure onto script-driven flow
refactor(pr-to-video)	restructure onto script-driven flow
refactor(faceless-explainer)	restructure onto script-driven flow
chore(skills)	refresh test-skills-fresh.sh workflow roster (6 → 10)

Depends on #1632 (frame-preset library + shared audio engine). These skills resolve ../../hyperframes-creative/frame-presets and ../../hyperframes-media/scripts/audio.mjs by path, so #1632 must merge first.

Why

The three workflows had each grown their own copies of capture/validate/prep/assembly logic. Moving them onto one script-driven backend means the authoring flow (storyboard → frames → audio → assemble) is shared, and each workflow keeps only what's genuinely specific to it (e.g. pr-to-video's ingest.mjs that folds gh PR artifacts into the synthetic capture package the shared backend reads).

How

• product-launch-video — build-frame token remix + caption tweaks; every frame authored as a directed shot; old bespoke scripts (captions/validate/prep/hoist/…) removed in favour of the shared lib.
• pr-to-video — ingest.mjs folds the two gh artifacts into the synthetic capture package; adds the mechanism beat (show behavior, not just the diff).
• faceless-explainer — every visual invented (typography / abstract graphics / diagram / data-viz), authored through the shared backend.
• test-skills-fresh.sh — roster updated to the current 10 workflows (adds website-to-video, embedded-captions, graphic-overlays, slideshow; drops the removed footage-recut) with refreshed example prompts.

Reconciled with upstream

• Skill names — fixed name: in both SKILL.mds to match their directories: pr-to-video-refactor → pr-to-video, faceless-explainer-refactor → faceless-explainer (and dropped a stale faceless-explainer-refactor mention in a pr-to-video/ingest.mjs comment).
• fix(skills): reject empty or partial scene files at assembly time #1629 guard — each assemble-index.mjs carries upstream fix(skills): reject empty or partial scene files at assembly time #1629's blank/partial scene-file guard, ported onto the restructured reader: before emitting data-composition-src, reject a scene file that exists but is empty / markup-less (!html.trim() || !/<\w/.test(html)) and tell the caller to re-dispatch that worker — so an interrupted worker fails loudly at assembly instead of cryptically at render.

Test plan

Skill content is .md / .html / .mjs / .json.

• lefthook pre-commit per commit — largefiles / oxfmt --check / commitlint clean.
• Each path verified byte-identical to the source branch except the six intended reconciliations (3× fix(skills): reject empty or partial scene files at assembly time #1629 guard, 2× skill-name fix, 1× stale-reference fix).
• No *-refactor residue anywhere in the three skills.
• scripts/test-skills-fresh.sh is the full install-and-verify harness; it asserts the complete workflow roster, so it lives here (the last PR of the series).

Branches off current main. Merge after #1632.

[jrusso1020]

Scope-down review per REVIEW_DISCIPLINE rule #4 (631 files / +8330/-87294 — net deletion is the per-skill-script removal claimed in the body). Posting as COMMENT — CodeQL is failing with 12 new high-severity alerts, which per REVIEW_DISCIPLINE rule #1 disqualifies an APPROVE on its own.

CodeQL — required check failing

CodeQL is FAILURE: "12 new alerts including 12 high severity security vulnerabilities." Pulling the alerts for refs/pull/1635/merge returns 263 entries (40 error-severity, 199 warning); the 12 new in this PR are a subset.

Two categories I traced:

1. In-scope (workflow-skill files) — 3 alerts, all js/incomplete-multi-character-sanitization:

• skills/product-launch-video/scripts/captions.mjs:227
• skills/pr-to-video/scripts/captions.mjs:227
• skills/faceless-explainer/scripts/captions.mjs:227

All three sites are out = out.replace(/<!--[\s\S]*?-->/g, "") — stripping HTML comments from a skin file. The CodeQL concern is the classic "incomplete comment sanitization" — /<!--[\s\S]*?-->/g doesn't handle nested or partial comment markers, so a payload like <!--<!---->--> leaves a dangling start-marker.

Read the actual code: skin comes from a caption-skin.html file shipped with each preset in hyperframes-creative/frame-presets/<preset>/caption-skin.html (PR #1632). So the input is internal content from the preset library, not attacker-controlled at runtime. The vulnerability is theoretical — a malicious preset could exploit it, but presets are codebase-shipped and gated by code review.

Still worth either:

• (a) Replacing the regex with a tighter loop (while (out.includes("<!--")) { ... }) so CodeQL stops flagging it, OR
• (b) Adding a one-line comment at the call site noting "input is preset-library content, not user-controlled — sanitization is comment-strip for lint-cleanliness, not XSS defense" so the suppress-CodeQL reasoning is visible.

The cleanest fix is the tighter loop — kills the alert + matches the actual defensive intent.

2. Out-of-scope — carryover from the upstream-squash-merge stack:

The branch contains commits with PR-suffix titles ((#1605), (#1553), (#1628)) that landed to main via squash-merge but with different SHAs. The branch carries the same content under different SHAs, and CodeQL's analysis of the PR ref includes alerts on those carried files (e.g. packages/core/src/runtime/bridge.ts:76,82, packages/studio/src/hooks/use*.ts, packages/core/src/generators/hyperframes.ts). These are NOT bugs introduced by this PR — they're alerts from upstream code that CodeQL is re-reporting via the PR ref.

Same shape as #1631 — git merge-base says clean-merge per git's recursive heuristic, but CodeQL doesn't dedupe by content equivalence.

Practical implication: of the "12 new" CodeQL counts, the 3 captions.mjs ones are this PR's; the other ~9 are upstream-carryover. The team's CodeQL gate may or may not let the merge through with upstream-carryover alerts — that's a team-discipline call, not a code-review call.

Body claims verified against the diff (per REVIEW_DISCIPLINE rule #3)

• "4 commits, 629 files" — confirmed: gh pr view returns 631 files (paginated; the gh ... --json files default truncates at 100 — I went through gh api ... pulls/1635/files --paginate to get the full count).
• "+8322 / −87291" — confirmed: +8330 / −87294, near-exact match. Net deletion is per-skill-script removal in favour of the shared backend, per the body.
• "Depends on #1632" — confirmed: paths in workflow skills (../../hyperframes-creative/frame-presets / ../../hyperframes-media/scripts/audio.mjs) resolve into #1632's content. Merge ordering MUST be #1632 → #1635.
• "#1629 guard ported to each assemble-index.mjs" — sampled skills/pr-to-video/scripts/assemble-index.mjs; the !html.trim() || !/<\w/.test(html) blank-scene guard is present. ✓
• Skill-name fixes (name: pr-to-video-refactor → pr-to-video, etc.) — sampled SKILL.md, confirmed names match directories. ✓

Foundation scope respected

The three workflow skills each now resolve hyperframes-creative + hyperframes-media by path. Verified skills/pr-to-video/scripts/audio.mjs shells out to the foundation engine; build-frame.mjs consumes presets via ../../hyperframes-creative/frame-presets/. No duplicated copies of the engine or preset library — good. ✓

Concerns

1. CodeQL alerts above — at minimum, the 3 captions.mjs:227 regex sanitizations are worth tightening before merge to clear the in-scope portion of the failure.
2. Stack ordering with #1632 — both PRs are open. They MUST land in order (#1632 → #1635) since #1635 references foundation paths. If a stamp lands on #1635 first, the merge will fail (foundation files don't yet exist).
3. Massive content I didn't read — the 631-file scope means I sampled rather than read in full. SKILL.md changes per workflow, ingest.mjs for pr-to-video, assemble-index.mjs per workflow, and test-skills-fresh.sh are the auditable surface I read. The other ~600 file changes (mostly deletions of per-skill scripts) I trusted to match the body's "removal in favour of shared lib" claim. If specific per-script content matters for correctness, a second reviewer focused on the deletion sweep would close that gap.

Stamp posture

Same as #1631 / #1632 — per the team's customer-partner-PR discipline (Miao is allowlisted but not a trusted-stamper), stamp eligibility routes through the team. From my read: needs CodeQL alerts addressed (or explicitly accepted on team-discipline grounds) + #1632 to merge first. Not stamping unilaterally.

Review by Jerrai

[miga-heygen]

Review: refactor(skills) — script-driven architecture migration

What this PR does

This is a major refactor, not a fix for the #1632 findings. It moves three narrated workflow skills (product-launch-video, pr-to-video, faceless-explainer) from bespoke per-skill scripts onto a shared "script-driven" architecture where TTS/BGM/SFX route through the shared hyperframes-media engine and frame authoring uses hyperframes-creative presets. Additionally: test-skills-fresh.sh is updated (6→10 workflows), a gsapDragCommit test import is fixed, and the router skill metadata is refined.

631 files changed, +8330/−87291. The net deletion is massive because each skill's old self-contained TTS/BGM/transcribe/provider logic (800+ lines each) is replaced by a thin ~215-line adapter that writes an audio_request.json and spawns the shared engine.

Relationship to #1632 bugs

The two bugs flagged on #1632 are indirectly resolved by elimination:

1. wait-bgm.mjs field mismatch (audioMeta.bgm_path/bgm_enabled vs. bgm.path nested / bgm_pending): wait-bgm.mjs is deleted from all three skills. The new adapter uses BGM mode: "retrieve" (synchronous retrieval, no detached background process), so there is no longer a wait step. The field mismatch ceases to exist because the old flat bgm_path/bgm_pending shape is replaced by a nested bgm: { path, volume, query, duration_s } object. This is a valid architectural fix — the mismatch was a symptom of the bespoke-scripts duplication.

2. heygenCredential() unguarded JSON.parse: The function is removed entirely from all three adapter scripts. Credential resolution is now the shared engine's responsibility. Whether the shared engine in hyperframes-media guards its own JSON.parse is outside this PR's scope (it's in #1632), but the per-skill copies of the vulnerable code are gone.

Both bugs are resolved in the sense that the buggy code no longer exists in these skills. The correctness now depends on the shared engine in #1632 being sound.

Code quality observations

Strengths:

• Clean separation of concerns: each skill's audio.mjs is now a thin adapter (~215 lines) that maps its domain model to the engine's neutral format and back. Much easier to reason about.
• The #1629 blank/partial scene-file guard is properly ported to the new assemble-index.mjs reader (!html.trim() || !/<\w/.test(html)).
• Skill name fixes (pr-to-video-refactor → pr-to-video, faceless-explainer-refactor → faceless-explainer) are good housekeeping.
• The gsapDragCommit test fix is a sensible inclusion (inherited main breakage).
• CI is green except CodeQL (which flags TOCTOU races and HTML sanitization — pre-existing patterns, not new to this PR).

Observations / questions:

1. Near-identical adapter scripts across three skills. audio.mjs (213-214 lines), assemble-index.mjs (181-183 lines), lib/storyboard.mjs (249 lines each), and lib/assets.mjs (55 lines each) are virtually identical across all three skills. The PR description says the refactor moves them "onto the shared backend" — but the adapter layer itself is copy-pasted. If a bug surfaces in parseStoryboard() or stageAssets(), it needs fixing in three places. Is there a plan to hoist these into a shared location (e.g. a hyperframes-skills-common/ package), or is per-skill duplication intentional for skill isolation/portability?

2. audio_engine_meta.json as a stable sidecar. The neutralPath() function writes the engine's output next to the skill's audio_meta.json. The comment says "so --only merges (generate then fetch-sfx) accumulate" — but I don't see merge logic in the adapter. runFetchSfx overwrites audio_meta.json entirely with toProductLaunchMeta(neutral), which would lose the voices from the earlier runGenerate step unless the engine itself merges into the neutral file. Is the engine responsible for that merge? If so, a comment clarifying "the engine merges into neutral across --only passes" would help future readers.

3. parseScript regex fragility. The (Frame N) pattern (/^#{2,3}\s+.*?\(frame\s+(\d+)\)/i) requires the exact parenthesized token in the heading. A SCRIPT.md heading like ## Introduction (frame 1) — hook works, but ## Frame 1: Introduction wouldn't. Presumably the upstream skill orchestrator controls the SCRIPT.md format, so this is fine — but a comment noting the contract would help.

4. CodeQL findings. The 9 CodeQL alerts (TOCTOU file-system races + incomplete HTML sanitization) are pre-existing patterns that this PR inherits, not introduces. Not blocking, but worth tracking.

Verdict

Well-scoped refactor that achieves a significant reduction in per-skill complexity. The deletion of wait-bgm.mjs and heygenCredential() from the skill layer resolves the two #1632 bugs by elimination. The main concern is the three-way duplication of adapter code — not a blocker, but worth a follow-up plan.

No blocking issues. I'd recommend approval once the audio_engine_meta.json merge-accumulation question (point 2) is clarified.

— Review by Miga

[jrusso1020]

Round-2 — verified the fixes.

fix(skills): tighten caption comment-strip + document audio --only merge (cfdc21312):

• Captions sanitization — buildFromSkin in all three workflows now strips comments in a fixpoint loop instead of a single global pass. Verified at skills/pr-to-video/scripts/captions.mjs:230 (and the matching sites in the other two workflows). Matches the suggested shape in my round-1 review and is the correct defensive intent — removing one comment can re-form a marker from a nested/partial pair (<!--<!---->-->), which a single pass misses. ✓
• Clears the 3 in-scope js/incomplete-multi-character-sanitization alerts.

fix(skills): remove existsSync->write TOCTOU in workflow scripts (cb6de92ee):

• 9 js/file-system-race alerts cleared via the right shape:
  • captions.mjs: caption-overrides shim → atomic writeFileSync({ flag: 'wx' }) (fail-on-exists is the actual race fix, not a recheck).
  • audio.mjs (sync-durations) + transitions.mjs (inject): dropped the existsSync precheck entirely, read directly, surface the same friendly error from a try/catch on readFileSync. No check→write gap.
• This is the right fix for TOCTOU — eliminate the check, not paper over it. ✓

CI: CodeQL PASS now (was 12 high-sev failures last round). The load-bearing blocker from round-1 is resolved.

Other concerns from round-1 still open:

• Stack ordering with #1632 — still strict; merge ordering must be #1632 → #1635.
• Massive content I didn't read — unchanged; same scope-down posture.

Stamp posture unchanged — routes through the team.

Review by Jerrai

[miguel-heygen]

Re-review after cfdc213 / cb6de92.

The CodeQL blockers from the earlier review are addressed: the three workflow captions.mjs files now strip comments in a fixpoint loop, the audio_engine_meta.json --only sfx merge behavior is documented at the adapter call sites, and the existsSync→write/read TOCTOU patterns were replaced with atomic create or direct read + catch. gh pr checks 1635 is now green at current head.

Remaining gate: stack order. #1635 references the foundation introduced by #1632, so #1632 must land first; do not approve/merge #1635 ahead of it.

Verdict: COMMENT (ready after #1632 lands; approval/stamp withheld until stack order is satisfied and authorized)
Reasoning: The technical blockers I checked are resolved and CI/CodeQL are green, but approving/merging before #1632 would violate the dependency order.

[miga-heygen]

Re-review (round 3) — post cfdc213 / cb6de92 / e882ce9 / 6192b5e

Four new commits since my last review. All address review feedback or fix rendering correctness. Verified each:

1. cfdc2131 — caption comment-strip + audio --only merge documentation

Captions: fixpoint loop (for (let prev = ""; prev !== out; )) is the correct shape — iterates until stable, handles the nested-marker corner case (<!--<!---->-->). Applied consistently across all three workflows. Clears the 3 in-scope CodeQL js/incomplete-multi-character-sanitization alerts.

Audio --only merge: my round-1 question about audio_engine_meta.json accumulation across --only passes is now answered directly in code comments: "the engine reads the existing neutral sidecar and recomputes only the sfx section, so voices/bgm from the earlier generate pass are preserved." The merge responsibility lives in the engine (in #1632), not in the adapter — the adapter correctly reads back the merged result via toProductLaunchMeta(JSON.parse(...)). This is the right split: adapter writes request, engine owns merge semantics, adapter reads back the unified output. Question resolved.

2. cb6de92e — existsSync→write TOCTOU elimination

Three patterns fixed across 9 files:

• captions.mjs: existsSync + writeFileSync → atomic writeFileSync({ flag: 'wx' }) with swallowed EEXIST. Correct — wx is exclusive-create, fails atomically if the file exists.
• audio.mjs (sync-durations): existsSync precheck on storyboard → direct readFileSync in try/catch. Same error message preserved.
• transitions.mjs (inject): same pattern for indexPath.

All three are the idiomatic fix — eliminate the check, let the actual operation surface the error. Behavior-preserving. Clears the 9 js/file-system-race CodeQL alerts.

3. e882ce97 — root composition ground color

Substantive rendering fix: per-frame roots are clip-gated by data-start/data-duration, so only the first frame's window overlaps global time 0. Frames after the first were rendering on bare body background (black). Fix: read frame.md canvas color via parseColors + semanticColors, paint it on #root as background. Falls back gracefully (null → no background rule, body letterbox shows through).

Note: this commit does use existsSync(framePath) — but this is NOT a TOCTOU issue. It guards an optional read path with no subsequent write to the same file, and the actual read is wrapped in try/catch anyway. The existsSync here is "skip optional enrichment if file is absent," not "check-then-write."

Good addition — this is a genuine rendering bug that would have surfaced in multi-frame videos with non-black backgrounds.

4. 6192b5e0 — drop router-tag edit

Restores skills/hyperframes/SKILL.md to avoid a guaranteed merge conflict with #1632 (which rewrites it wholesale). Correct housekeeping — the tag edit belongs in the foundation PR.

CI status

All checks green, including CodeQL (was 12 high-severity failures in round 1). Build, test, lint, format, typecheck, CLI smoke, SDK contract, studio load smoke, render on Windows, preview parity — all pass.

Open items from earlier rounds

Concern	Status
CodeQL (3 sanitization + 9 TOCTOU)	Resolved — fixpoint loop + atomic/direct-read
audio_engine_meta.json merge-accumulation	Resolved — documented; engine owns merge
Stack ordering #1632 → #1635	Still applies — #1632 must land first
Three-way adapter duplication	Unchanged — not blocking, follow-up item

Verdict

All technical blockers from rounds 1-2 are resolved. CI is fully green. The only remaining gate is merge ordering: #1632 must land before #1635.

Ready for approval once #1632 merges.

— Review by Miga