fix(product-launch): hoist approved frame videos during assembly

[miguel-heygen]

Verified behavior

The product-launch assembler does enforce the runtime boundary: media nested in a frame sub-composition is not driven reliably, while the host root can drive direct-child media. A minimal assembly repro with a timed muted frame video failed the old guard with the sub-composition media error.

Fix

• Frame workers may declare an explicitly approved timed video with data-frame-video="approved", data-start, data-duration, and data-track-index.
• assemble-index.mjs removes that declaration from the frame file and hoists the video to the host root, translating frame-relative start to global time and assigning an isolated track lane.
• Audio remains orchestrator-owned; unmarked frame video/audio remains a hard failure. Static still fallback remains optional creative guidance, not a forced runtime workaround.
• Nested/template or direct-subcomposition media is not silently converted to stills.

Evidence and tests

• media-contract.test.mjs runs the real assembler against a minimal frame and asserts root-level video timing/track output plus removal from the frame sub-composition.
• The same test asserts the worker guidance documents the approved-video contract and audio ownership.
• node --check skills/product-launch-video/scripts/assemble-index.mjs
• node --test skills/product-launch-video/scripts/media-contract.test.mjs (2/2)

Head: d65079058

[vanceingalls]

Verdict: 🟢 LGTM at 6ad49f87

Tight patch — a 60-line assembler-side change that converts a hard-fail into an explicit, orchestrator-approved hoist pathway, keeps the audio boundary intact, and adds a real behavioral assembler test. The scope discipline (audio still hard-failed, unmarked media still hard-failed, static-still fallback still optional) is right.

Mechanism verified:

• hoistApprovedVideos at skills/product-launch-video/scripts/assemble-index.mjs — <video> scan regex is <video\b((?:[^>"']|"[^"]*"|'[^']*')*)>([\s\S]*?)<\/video\s*>, i.e. attribute-capture respects quoted values inside the tag. The approval gate reads data-frame-video === "approved" first, then requires finite data-start, positive data-duration, finite data-track-index — else emits a label: error and leaves the video in place (which then trips the existing ② hard-fail downstream — good, no silent bypass on malformed declarations).
• guardFrame — pre-populates repairedHtml with approved.html when the hoist mutated the source, so the sub-comp file gets written with the video removed even when no dim-inject repair happens. If dim-inject also runs, it splices further into approved.html. If neither happens, repairedHtml stays null. Three branches, all correct.
• Global-time translation at the mount step — globalStart = r3(m.start + video.start) puts frame-relative time in world time; track lane 1000 + frameIndex * 100 + video.track reserves a per-frame band well above the assembler's own numbered lanes.
• Frame-worker sub-agent instructions carry the contract precisely: <video data-frame-video="approved" data-start="..." data-duration="..." data-track-index="...">, audio orchestrator-owned, no unapproved URLs, no timing-less videos.

Test coverage:

• media-contract.test.mjs:20-44 — real assembler spawn, temp project, minimal frame with data-frame-video="approved", asserts host index.html receives the video with translated data-start="0.25" / data-duration="1.5" / data-track-index="1007" and the frame file is emptied of <video>. Behavioral test, not a mock — matches the assembly path end-to-end.
• media-contract.test.mjs:11-18 — pins the frame-worker.md contract text so the docs and the assembler stay in lockstep.

Observations, non-blocking:

• Regex runs on original html, not on sanitizedForScan. hoistApprovedVideos executes before the comment/script/style blanking that guardFrame uses for ②/③. A frame file with <!-- example: <video data-frame-video="approved" data-start="0" data-duration="1" data-track-index="0"></video> --> in a comment, or with the exact declaration inside a <script> string literal, would false-positive-hoist. Likelihood is low in practice (workers writing "commented-out approved video examples" or JS string demonstrations of the contract), but it's the same bug class you just fixed at the lint layer in #2223 — the regex sees inside quoted-attribute/comment/script contexts because it doesn't understand outer structure. If you want the assembler to inherit the same discipline, you'd either (a) run hoistApprovedVideos on the sanitized copy and splice positions back into the original, or (b) do the tag walk with the new parseHtmlStructure primitive from #2223 once it lands, which handles comment/script/style scoping natively.
• Track lane math implicitly caps video.track at 99. 1000 + frameIndex * 100 + video.track — frame-i's video.track=100 collides with frame-(i+1)'s video.track=0. Realistic worker traffic is 0-1 tracks per frame, so unlikely to hit; a defensive if (video.track >= 100) fail-loud (or a + frameIndex * 1000 for stricter isolation) would rule it out.
• cleanedAttrs doesn't strip class. If a frame worker writes <video data-frame-video="approved" class="clip decorative" ...>, the emitted tag is <video ... class="clip decorative" ... class="clip" ...> — HTML allows only one class attribute and most parsers take the first, so the explicit class="clip" here becomes decorative. The frame-worker.md instructions don't show class in the approved-video declaration, so authors likely won't hit this — but a .replace(/\sclass\s*=\s*(?:"[^"]*"|'[^']*')/i, "") next to the other four strips would harden it.
• Test suite could grow one edge case: an approved video with a missing required attr (asserts the label-prefixed error surfaces + video stays in place → downstream ② hard-fails). Current tests cover the happy path; the failure-mode assertion would lock the "no silent bypass on malformed" contract in.

CI: current head 6ad49f87 has Skills: manifest in sync ✅, Format ✅, Test: skills ✅ — the prior head's format + stale-manifest failures were addressed in this push. Change-detection routed most other checks to skipped (skills-only PR touches don't need Studio/SDK/CLI lanes). Merge-blocking gates satisfied.

R1 by Via

[miguel-heygen]

Review follow-ups addressed in be38ac685: approved-video detection now sanitizes comments/scripts before hoisting, generated root videos always receive a stable id, class attributes are normalized, and track-lane isolation uses a 1000-wide frame stride. Added a negative behavioral test proving declarations hidden in comments/scripts are not hoisted.

The direct assembler/render smoke also completed with a local 2s MP4 fixture: hyperframes lint passed 0 errors/0 warnings and hyperframes render -o out.mp4 produced the output artifact. CI is green.

[james-russo-rames-d-jusso]

Reviewed at be38ac6.

Round context. Magi's ask was on d65079058; head has since advanced through two hardening commits (bc69f495 test(product-launch): harden approved video hoist + be38ac68 chore: refresh product-launch skill manifest). Reviewing against current HEAD be38ac685. Miguel-authored (verified .author.login === "miguel-heygen" across all 5 commits, no bot trailers) — normal HF stamp routing applies.

Hardening progress since Via's earlier observations: the class-conflict on emit is fixed (class now stripped from cleanedAttrs at L199), track stride widened to 1000/frame (L426) so a worker declaring data-track-index up to 999 no longer collides with the next frame's low tracks, and hoisting is now scoped to real HTML positions via the comment/script-blanked scan[offset] !== "<" guard at L178 (with a matching negative test in media-contract.test.mjs). Clean tightening.

What still bothers me (both inlined):

1. Silent 0-default on missing/unquoted admission attrs — the quoted-only attrValueFrom regex + Number(null) === 0 combine to let a worker's forgotten-or-unquoted data-start / data-track-index pass silently with value 0. data-duration is saved by the <= 0 check; the other two aren't. Verified empirically. Real-world scenario: LLM-authored worker forgets quotes or misses an attr, video hoists at wrong t.
2. Arbitrary attribute passthrough — everything not in the 5-item strip list (now including class) rides through, including on* handlers. The "approved" admission gate implies a trust boundary; if it does, event handlers should not survive.

Neither is a "you can't ship this" — they're both worker-mistake / worker-hostile edge cases. But the fixes are ~10 lines each and lock the contract.

Non-blocking observations:

• Coverage gap. Tests cover happy path + comment/script-hidden negative. Add: (a) missing admission attr → expect error, (b) unquoted admission attr → contract choice + test, (c) <video> with onerror → attr does/doesn't survive per fix. Aligns with the media-contract.test.mjs naming intent.
• attrValueFrom (L164) doesn't regex-escape ${name}. Safe today (all 4 callers pass hardcoded names with -), a footgun if someone adds a caller with a . or [] in the name. name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") if you want to close it.
• hoistedVideos on guardFrame's return shape isn't declared in the leading JSDoc. Small doc drift.

CI: 16 pass, 22 skipping (change-detection skips downstream on file-scope-only changes), 1 pending — effectively green from the required-check angle.

— Review by Rames D Jusso

[vanceingalls]

Verdict: 🟢 LGTM-conditional at be38ac68

My R1 items are cleanly addressed; endorsing Rames-D's two additional findings (his R1) as prerequisites — both are silent-worker-mistake shapes I missed. Verdict conditional on those landing.

My R1 items — per-finding disposition

✅ Observation 1 — Regex scope-blindness on original html (fixed).

hoistApprovedVideos now builds a scan copy with <!--...-->, <script>...</script>, and <style>...</style> regions replaced by same-length space runs, then guards inside the html.replace(re, ...) callback with if (scan[offset] !== "<") return full;. Elegant — same-length blanking preserves offsets so offset from html.replace remains a valid index into scan, and a <video whose opening < lands in a blanked region bails without touching the original html. Verified for all three false-positive shapes (comment-hidden, script-string-hidden, style-hidden). Behavioral negative test at media-contract.test.mjs:46-65 locks the sanitize+guard combination in.

✅ Observation 2 — Track lane math implicit cap (fixed).

1000 + frameIndex * 100 + video.track → 1000 + frameIndex * 1000 + video.track. Per-frame band widened from 100 to 1000, so collision now requires video.track >= 1000 (effectively unreachable for realistic worker traffic). Existing test's data-track-index="1007" assertion still holds (frameIndex=0, video.track=7 → 1000+0+7 = 1007, unchanged).

✅ Observation 3 — cleanedAttrs doesn't strip class (fixed).

Added .replace(/\sclass\s*=\s*(?:"[^"]*"|'[^']*')/i, "") to the strip list. If a worker writes class="clip decorative", the decorative bit is dropped and the emitted tag has exactly one class="clip". Consistent with the assembler's clip-contract normalization role.

✅ Observation 4 — Negative-case test for hidden declarations (added).

media-contract.test.mjs:46-65 writes a frame with two hidden approved-video declarations (script-string + comment) and asserts data-track-index="1001" doesn't appear in the emitted index. Behavioral coverage.

Small strengthening, optional: the assertion checks against data-track-index="1001" (script-hidden variant) but the fixture also seeds a comment-hidden variant with data-track-index="2" (would emit "1002" if hoisted). Both variants pass through the same scan[offset] guard, so functionally equivalent — but assert.doesNotMatch(index, /<video\b/i) on the emitted index would lock the "no hoist from hidden declarations, at all" contract in one line and cover both explicitly.

Endorsement — Rames-D's R1

Both his findings are legitimate real-world worker-mistake shapes I under-scoped in R1:

• Silent 0-default on missing/unquoted admission attrs. Verified empirically: attrValueFrom returns null for missing (or unquoted) attrs, Number(null) === 0, Number.isFinite(0) === true — so a worker who writes data-frame-video="approved" + data-duration="1" but forgets data-start OR data-track-index slips through with the missing attr defaulting to 0. duration is saved only because of the <= 0 check, which doesn't apply to start (0 IS a legit start) or track (0 IS a legit lane). The correct shape is raw === null → error before the numeric check, per Rames' fix sketch — cleanly separates "worker forgot" from "worker legitimately meant zero." Worth flagging: the PR body says "must declare finite data-start, positive data-duration, and data-track-index" — that promise isn't fully honored by the current check.
• Arbitrary attribute passthrough (on* handlers). The "approved" admission gate reads as a trust boundary — a worker asserting the video is safe to hoist to host root. If the gate is a trust boundary, <video onerror="..."> or <video onload="..."> surviving the 5-item strip list crosses it. The strip list needs to either allowlist known-safe attrs (src, muted, poster, loop, etc.) or explicitly strip on* handlers (.replace(/\son[a-z]+\s*=\s*(?:"[^"]*"|'[^']*')/gi, "")). Rames' framing (trust-boundary contract) is the right lens.

Rames' third suggestion (test coverage: missing-attr / unquoted-attr / on-handler pass-through) follows naturally from the two fixes and covers the same ground my "negative-case coverage" nit gestures at. His attrValueFrom regex-escape and JSDoc-drift notes are additional small tightenings, both correct.

Verdict recap

• R1 findings from Via: ✅ all four addressed.
• R1 findings from Rames-D: 2 legitimate, prerequisite-to-merge.
• CI: Skills: manifest in sync ✅, Format ✅, Test: skills ✅ on current head. Only in-progress: Analyze (javascript-typescript) (CodeQL, non-gating for skills-only).

Ship after Rames' silent-0 + on* items land.

R2 by Via

[vanceingalls]

R3 addendum at 70ef2d02 — Rames-D items now landed

Race-cite correction on my R2 (posted moments ago citing be38ac68): Miguel already pushed the fix commit in parallel. Verifying against current head 70ef2d02:

✅ Silent 0-default on missing admission attrs (Rames R1) — fixed.

New guard at hoistApprovedVideos:

const rawStart = attrValueFrom(attrs, "data-start");
const rawDuration = attrValueFrom(attrs, "data-duration");
const rawTrack = attrValueFrom(attrs, "data-track-index");
if (rawStart === null || rawDuration === null || rawTrack === null) {
  errors.push(
    `${label}: approved frame video must declare quoted data-start, data-duration, and data-track-index`,
  );
  return full;
}

Null-raw check runs before the numeric check — a worker who forgets any of the three (or writes it unquoted) trips the label-prefixed error and the video stays in place → downstream ② hard-fails. New negative test at media-contract.test.mjs "rejects an approved video with missing admission timing" locks it via assert.notEqual(result.status, 0) + assert.match(result.stderr, /must declare quoted data-start/i).

✅ Arbitrary on* handler passthrough (Rames R1) — fixed.

cleanedAttrs chain now includes .replace(/\son[a-z]+\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]+)/gi, ""). Case-insensitive, tolerates unquoted values. The strip runs globally so multiple handlers (onerror + onload + …) all go. Positive test fixture now includes onerror="alert(1)" and asserts assert.doesNotMatch(index, /onerror=/i) on the emitted host index.

✅ Extras Miguel added on top:

• srcdoc also stripped (.replace(/\ssrcdoc\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]+)/gi, "")) — defense-in-depth so a hoisted video can't carry embedded HTML markup into the host root through the srcdoc attribute. Not on either of our lists; correct to add.
• attrValueFrom regex-escapes the name argument (name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")) — closes Rames' small footgun observation for any future callers with ./[] in attr names.

CI: current head still holds Skills: manifest in sync ✅, Format ✅, Test: skills ✅.

Revised verdict: 🟢 LGTM at 70ef2d02. Ship it.

R3 by Via (addendum)

[james-russo-rames-d-jusso]

Re-reviewed at 70ef2d02 — verifying the two findings from my first pass.

Both fixed cleanly:

• 🔴 admission-attr silent-0 — assemble-index.mjs:181-187 now reads each data-start / data-duration / data-track-index as raw*, explicitly checks === null, and pushes a clear error before any Number(…) conversion. Missing OR unquoted attrs both reject now instead of coercing to 0 via Number(null). Locally re-ran the missing-attr and unquoted-attr fixtures against the extracted hoistApprovedVideos; both produce the error and leave the tag in place.
• 🟠 event-handler passthrough on admission attrs — assemble-index.mjs:210-211 adds on* (with the unquoted-value fallback [^\s>]+, so onclick=badFn also strips) and srcdoc to the cleanedAttrs strip. Verified locally: onerror="alert(1)", unquoted onclick=badFn, and srcdoc="…" all disappear from the emitted root tag.

Nice bonus catch: attrValueFrom now escapes the attr name (name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")) — the follow-up I called out non-blocking is already in.

Small non-blocking observations, in case worth a follow-up:

• Attribute-name allowlist would be sturdier than a blocklist over time. The strip covers on* + srcdoc + the 5 admission-side attrs; anything else on the hoisted tag (e.g. a style="background:url(…)", a rogue poster pointing at a data-URI, future custom event-adjacent attrs like nonce, formaction, etc.) forwards verbatim. Not a blocker for this PR — this is trusted worker output — but if the boundary is ever exercised by less trusted callers, flipping to "only forward src / preload / muted / playsinline / ARIA" is easier to reason about than growing the deny-list.
• Regression tests. The new test(product-launch): harden approved video hoist covers the class-normalization + track-stride + comment/script cases from the prior turn. The fix(product-launch): validate and sanitize hoisted video attrs commit itself amends media-contract.test.mjs (+23 lines) — worth confirming those explicitly assert both the null-rejection error AND the stripped-handler + stripped-srcdoc emitted output (didn't crack open the test diff in this pass).

CI is clean from where I sit (skills + skills-manifest + preview-regression + player-perf + regression + windows-render all green or skipping non-blocking).

LGTM from my side.

— Review by Rames D Jusso