fix(capture,audio,docs): defects found running product-launch-video end to end

[WaterrrForever]

Rebased onto main now that #2880 / #2881 / #2882 have landed. Everything here came out of running the full product-launch-video workflow twice against a real site (linear.app) to verify those three PRs — pre-existing defects the runs walked into, plus the two wording follow-ups that could only be written once their sentences were on main.

Three defects

Scraped SVGs are not usable as .svg files. assetDownloader wrote an inline <svg>'s outerHTML straight to assets/svgs/*.svg. An inline SVG inherits its namespace from the HTML parser, so outerHTML omits xmlns — fine pasted back into HTML, not a standalone document. <img src="logo-abc123.svg"> renders a broken-image icon, which is exactly how these assets get consumed. Observed live: a frame worker got a broken wordmark and had to inline the path data by hand.

toStandaloneSvg declares the namespace on the way to disk, plus xmlns:xlink but only when an xlink: attribute is actually used. The filename hash moved to the bytes that land on disk so it still cannot drift from content. Idempotent; root attributes and path geometry untouched.

sfx: none became a cue named "none". fetch-sfx split the storyboard's sfx: list and dropped only empty strings, so the absence marker reached the audio engine as a real cue that could not resolve. The absence spellings (none / no / n/a / na / skip / - / — / –, case-insensitive) are part of the storyboard vocabulary; they are filtered out now, and a mixed list keeps its real cues.

bgm_pending was lost translating neutral meta to product-launch meta. A detached Lyria/MusicGen generate leaves bgm: null, bgm_pending: true until the track lands, but toProductLaunchMeta returned only {bgm, voices, sfx}. "Not ready yet" became indistinguishable from "silent by design" — and since fetch-sfx rewrites audio_meta.json from the sidecar, a still-generating bed got snapshotted away with nothing to signal it. Observed live: the run headed toward a silent film while the storyboard claimed a music bed, and the agent only caught it by noticing the mismatch itself. The flag survives now, and fetch-sfx warns when it snapshots a pending bed.

The scroll-shot plate

A scroll shot needs one continuous tall image to slide a viewport down. Capture emitted no such file — only 15 viewport-sized scroll-position tiles, which tiles cannot substitute for.

A full-page.png did exist and was dropped in 62b5517: 1/8 agents read it, contact sheet covered the same ground. That measured it as a comprehension artifact, on an eval where nothing was building scroll shots. A scroll shot is a different consumer, so the plate comes back — with the two properties that make it worth having:

• Taken last, after the scroll traversal, so lazy images have loaded and scroll-triggered reveals have fired. A plate shot on arrival is full of blank bands — a good reason to look once and never again.
• Sticky chrome neutralised, because fullPage bakes a fixed header in at one position and freezes a nav across the middle of the plate. The viewport tiles keep sticky deliberately (natural browsing state); the plate cannot. Positions are recorded and restored in a finally, so later extraction passes see an unmodified DOM.

1x, deliberately. A 1920-wide plate is pixel-exact for a 1920×1080 viewport travelling down it. 2x is what you'd want to push in without softening text, but doubling a long marketing page passes Chrome's 16384px screenshot cap precisely on the pages that most want a scroll shot (linear.app: 10962 CSS px → 21924 at 2x). A frame needing that headroom captures its own region at 2x instead. Pages over the cap get no plate rather than a silently clipped one, and the caller falls back to the tiles.

Two wording follow-ups

#2881 pointed the scroll shot at an artifact that did not exist — "use a 2x full-page capture and animate the viewport over it". Both runs watched the agent go looking, not find it, and improvise (once by re-capturing 2x strips per section, once by using the native tiles full-bleed). The sentence now names the plate this PR adds, its absence on pages too tall to capture in one piece, the tile fallback, and why pushing in past 1:1 still wants a region capture.

#2880's constant fields were being read as absent ones. It asks for x/y, scale, opacity and direction/speed on every handoff. Across two runs on the same model, opacity went 0/12 then 12/12 — when the value never changes, leaving it out is a reasonable reading. But downstream an omission and "there is no handoff here" are the same thing, so the field set is now stated as binding even when constant, on both the orchestrator's and the worker's side of the contract.

Not included

assemble-index.mjs rewrites index.html wholesale and so discards the block transitions.mjs inject wrote — any Step 6 rework silently loses transitions. Fixing it means deciding whether assemble preserves an injected block or inject becomes re-appliable; it touches both scripts and the Step 5/6 ordering in SKILL.md.

SFX cues cannot be offset within a frame (they fire at frame start), which cost one run a click it wanted mid-frame. That is a feature request, not a defect.

Validation

• bunx vitest run src/capture — 90 pass (10 new)
• node --test skills/product-launch-video/scripts/audio.test.mjs — 13 pass (5 new)
• bun run lint:skills · oxlint + oxfmt clean · tsc --noEmit clean · git diff --check clean

Eval report: https://www.heygenverse.com/a/product-launch-video-three-prs-all-merge-dbd20ff0-7d30-4647-9a63-32fa9e1db697

[miga-heygen]

1. toStandaloneSvg — SVG namespace fix

Correct. The regex chain does exactly what it says:

• /<svg\b[^>]*>/i finds the opening tag (\b prevents matching <svgfoo>). ✓
• /\sxmlns\s*=/i on the tag only — avoids false positives from child attributes. ✓
• /\sxlink:[a-z-]+\s*=/i on the full outerHTML — correct, since xlink: attributes live on children (<use xlink:href="...">). ✓
• tag.replace(/^<svg\b/i, ...) inserts declarations right after <svg. ✓

Idempotent (early return when add.length === 0). Non-SVG input returned unchanged. Hash now computed on the standalone bytes (post-conversion), so filename matches content. One call site (downloadAssets inline SVG loop). 5/5 tests cover the important cases.

---

2. SFX_NONE filtering

Clean fix. SFX_NONE Set covers the full storyboard vocabulary: none, no, n/a, na, skip, -, — (em dash), – (en dash). Case-insensitive via .toLowerCase().

Filter chain .filter((s) => s && !SFX_NONE.has(s.toLowerCase())) correctly: (a) drops empty strings, (b) drops absence sentinels. Mixed list "whoosh, none, click" → ["whoosh", "click"]. ✓

One definition site, one consumer. 3 tests cover core cases.

---

3. bgm_pending preserved in toProductLaunchMeta

Exact fix. !!neutral.bgm_pending correctly coerces (undefined → false, true → true). Warning fires only when meta.bgm_pending && !meta.bgm — genuinely relevant. console.warn goes to stderr. ✓

2 tests: pending+no-bgm → flag preserved + warning; resolved bgm → pending false + no warning. Both verify JSON output AND stderr. Complete.

---

4. captureFullPagePlate — scroll-shot plate

Well-designed reintroduction addressing both reasons the original was removed:

1. Taken AFTER scroll traversal — placed at end of captureScrollScreenshots. Lazy images loaded, scroll-triggered reveals fired. ✓
2. Sticky/fixed neutralized — querySelectorAll('*') + getComputedStyle finds fixed/sticky elements, saves original in data-hf-plate-position, restores in finally. ✓

• MAX_PLATE_HEIGHT_PX = 16384 — Chrome's Skia limit. Pages over the cap get null. ✓
• Deliberately 1x — no setViewport call. 2x blows past Skia cap on exactly the pages that want scroll shots. ✓
• finally ensures restoration even on screenshot error. Test explicitly verifies this. ✓
• Sits inside the existing non-critical try/catch — plate failure doesn't lose viewport screenshots. ✓

One follow-up suggestion: if page.evaluate in the finally block itself throws (browser crash), the original error is masked (standard JS finally behavior) and the page is left with sticky elements stuck as static. Wrapping the restoration in a defensive try/catch would be strictly safer:

finally {
  try {
    await page.evaluate(\`...restoration...\`);
  } catch {
    // Restoration failed — log but don't mask the original error.
  }
}

Not blocking — the restoration is a simple DOM query that shouldn't fail on a live page.

5/5 tests including call-order verification (invocationCallOrder proves neutralize < screenshot < restore). Nice.

---

5. Skill wording

Both changes accurate against the code:

• SKILL.md: Path capture/screenshots/full-page.png matches code. 1x rationale, pixel-exact at 1920, fallback to tiles when absent, 2x-for-push-in — all consistent with implementation. ✓
• frame-worker.md: "a field the packet states as unchanged is still binding, not optional" — mirrors SKILL.md from the worker's perspective. No drift. ✓

---

CI

50 pass, 18 cancelled (old run superseded by rebase). No signal from the cancellations.

---

Verdict

Approve. Four real bugs fixed with clean, well-tested code. Each fix is self-contained with a single call site. The skill docs accurately reflect the code changes. The scroll-shot plate reintroduction is well-reasoned — the post-traversal timing and sticky neutralization address exactly why the original was dropped.

-- Miga

[jrusso1020]

Read all nine changed files end to end at 37961e36c, plus three unchanged files the modified contracts reach into.

Additive to @miga-heygen, which covered the in-diff correctness of all four fixes thoroughly — the regex chain, the sentinel set, the !! coercion, the 1x/cap rationale, and both doc sentences. I'm not restating any of it, and I independently reached the same read on CI (the cancelled runs are superseded, replaced ~50s later; every workflow has a success at this head, so BLOCKED is the reviewer gate alone).

My findings are in one specific gap: that review concludes each fix is "self-contained with a single call site." That holds inside the changed files. It does not hold at the contract level, and the three items below are what the per-site audit turns up.

Strengths

• assetDownloader.ts:64-65 — hashing the post-transform bytes rather than the raw outerHTML keeps the content-addressed filename honest. Inserting a transform in front of an existing hash is exactly where that invariant usually breaks.
• screenshotCapture.test.ts:73 — the throw path gets its own test asserting the restore still ran. That's the half of a finally that normally ships unpinned.
• audio.test.mjs:183 — the mixed list (whoosh, none, click) is the case that distinguishes "filter the sentinel" from "drop the whole line". A sentinel fix that only tests the all-sentinel input can't tell those apart.

Body claims I checked against source rather than taking them: namespaces are the correct http://www.w3.org/2000/svg (assetDownloader.ts:37) and http://www.w3.org/1999/xlink (:39); the new-test counts match the diff (5 + 5 under src/capture, 5 in audio.test.mjs); and both corrected sentences match the code they describe.

important — both audio defects also exist, unfixed, in the sibling copy of this same audio model

skills/faceless-explainer/scripts/audio.mjs carries both bugs verbatim:

• its toProductLaunchMeta returns { bgm, voices, sfx } — bgm_pending dropped identically
• its cue parse ends .filter(Boolean) — no absence sentinels

Grepping that file for bgm_pending or SFX_NONE returns nothing at all. That skill's own SKILL.md describes this as an audio model "shared across the workflows that reuse this audio model" and documents music: none as canonical vocabulary — so a storyboard there spelling a frame sfx: none walks into the same cue-literally-named-"none" failure this PR just fixed, and a detached generate there loses the same flag.

Not introduced here and outside the stated end-to-end scope, so I'm not gating on it. But it's the same bug class one directory over, both fixes are a handful of lines, and the reasoning in these commit comments transfers unchanged. Worth carrying across now while the context is loaded rather than rediscovering it from a second live run.

important — the flag survives the snapshot, then the assembler drops it again

Keeping bgm_pending through toProductLaunchMeta plus the fetch-sfx warning fixes the snapshot boundary. But skills/product-launch-video/scripts/assemble-index.mjs:479 rebuilds the object as { bgm: parsed.bgm ?? null, voices: ..., sfx: ... }, dropping the flag a second time, and that file has no pending handling anywhere.

So at the step that actually produces the film, "not ready yet" is still indistinguishable from "silent by design" — this PR's own framing of the defect.

The fetch-sfx warning does mitigate it within one continuous run. It doesn't cover the case that motivated the fix: assemble is re-run on Step 6 rework, typically long after that line scrolled past, and it will happily assemble a silent film from a snapshot whose own JSON says bgm_pending: true. Carrying the flag through and warning (or refusing) on bgm_pending && !bgm at assemble time closes it at the consumer that matters.

important — the plate's height guard is computed from a measurement the design guarantees is stale

captureFullPagePlate cap-checks the scrollHeight measured at screenshotCapture.ts:164, which runs before the scroll traversal. The plate is taken at :215, after it. Two things grow the document in between:

1. Lazy images loading during the traversal — which is the stated reason to take the plate last.
2. The neutralise pass itself: moving fixed elements to static puts out-of-flow boxes back into flow.

So the guarantee in the comment — pages over the cap get no plate rather than a silently clipped one — doesn't hold on the pages it's aimed at. Something measured at 15000 can cross 16384 by capture time. And a clipped plate is undetectable downstream: SKILL.md teaches the agent to fall back when the file is absent, not when it's present but truncated, which is the quieter of the two failures.

Re-measuring inside the function after the neutralise and immediately before page.screenshot is a one-line change that makes the check mean what it says.

Blast radius is bounded, which is why this isn't a blocker: an over-cap fullPage either throws — swallowed by the enclosing non-critical catch, leaving no plate, which is the intended outcome anyway — or clips.

Separately, and only because it's easy to read the other way: the plate is not scoped to scroll shots. captureScrollScreenshots runs on every capture, so every capture now takes an extra full-page screenshot. That's once per site at capture time, and the earlier removal was a comprehension judgment rather than a cost one, so this reads fine to me — just worth stating that it is unconditional rather than scroll-shot-gated.

nit — a > inside a root-<svg> attribute makes toStandaloneSvg emit a duplicate xmlns

/<svg\b[^>]*>/ stops at the first >, and HTML serialization does not escape > inside attribute values. On <svg data-a="x>y" xmlns="http://www.w3.org/2000/svg"> the captured "tag" truncates to <svg data-a="x>, so the \sxmlns\s*= test runs against a fragment that excludes the real declaration and adds a second one. Two xmlns attributes are a fatal error in a standalone document — the exact scenario this function exists to produce. Insertion is anchored at ^<svg\b, so the attribute value itself survives; only the detection is fooled.

Same line: outerHTML.replace(original, tag) treats tag as a replacement pattern, so a literal $& in a preserved attribute would corrupt the output. Splicing at the match index sidesteps both. Rare inputs, hence nit.

nit — currentColor still won't render standalone

The idempotency fixture at assetDownloader.test.ts:118 is fill="currentColor", which is representative of scraped wordmarks. Standalone, currentColor resolves against the file's own initial color — black — so a mark that inherited its brand colour from page CSS now parses as a valid document and renders in the wrong colour through <img src>.

Genuinely beyond "declare the namespaces", and the broken-image symptom that motivated the fix is properly gone. Flagging only because the stated goal is usable-as-a-.svg, and a black wordmark fails more quietly than a broken-image icon.

Verdict: APPROVE
Reasoning: Three real defects fixed at the right layer, with tests that pin the failure modes rather than the happy path, docs corrected to match the code, and CI genuinely green at this head. Everything above is either pre-existing in a file this PR doesn't touch or a narrowing of a new guard — none of it introduced here, and none worth holding three good fixes for.

— Rames Jusso

[miguel-heygen]

Additive review — Miga covered the in-diff implementations and Rames identified the wider contract gaps. The focused capture and audio suites pass locally, and the latest exact-head CI set is green.

Specific strengths:

• packages/cli/src/capture/assetDownloader.ts:31 makes inline SVG serialization idempotently standalone, and packages/cli/src/capture/assetDownloader.ts:64 correctly hashes the bytes written.
• packages/cli/src/capture/screenshotCapture.ts:70 restores mutated DOM state in finally; the new tests cover success, over-height skip, ordering, and screenshot failure.

Blockers:

• [blocker] The shared audio contract is only fixed in one workflow. skills/product-launch-video/scripts/audio.mjs:118 preserves bgm_pending and skills/product-launch-video/scripts/audio.mjs:203 filters absence sentinels, but the sibling shared implementation still returns { bgm, voices, sfx } at skills/faceless-explainer/scripts/audio.mjs:115 and still uses .filter(Boolean) at skills/faceless-explainer/scripts/audio.mjs:201. A faceless storyboard with sfx: none therefore still emits a cue named none, and its detached bed still loses bgm_pending. Update the sibling implementation/tests or centralize the translator/parser so every consumer of this contract gets the fix.
• [blocker] The final assembly boundary still turns pending BGM into silence. skills/product-launch-video/scripts/assemble-index.mjs:479 drops bgm_pending while parsing audio_meta.json. The warning added by fetch-sfx is useful, but the operation that builds the film still cannot distinguish “not ready yet” from “silent by design.” Refuse assembly while bgm_pending && !bgm (or otherwise enforce that state) and pin it with a test.
• [blocker] The plate height guard uses a stale measurement. packages/cli/src/capture/screenshotCapture.ts:164 measures before the traversal that deliberately loads lazy content, then packages/cli/src/capture/screenshotCapture.ts:215 passes that old value to the new guard. Neutralizing fixed/sticky elements can also change document height. Re-measure after traversal and neutralization before screenshotting, while keeping restoration inside finally; add a test where the initial height is under the cap but the final height is over it.

Verdict: REQUEST CHANGES
Reasoning: The implementations in the changed sites are solid, but the same audio contract remains broken at sibling/final consumers, and the new plate can bypass its own clipping guard on dynamically expanding pages.

— Magi

[WaterrrForever]

All three merge-blockers addressed in 194fb69, plus Miga's finally suggestion. Rames and Magi were both right on all three; the plate one was self-inflicted by this PR, so thanks for catching it before it shipped.

Plate guard checked a stale height. captureFullPagePlate now measures the document itself at call time instead of taking the caller's pre-traversal number — the whole design shoots the plate after lazy content loads, so the value it was trusting read low on precisely the long pages the guard exists for. And since the capture can trigger another round of loading, it also reads the produced PNG's IHDR height before writing and emits nothing if Chrome came back over the cap. A clipped plate can no longer reach disk. Signature dropped its scrollHeight parameter, so the stale-input shape is gone rather than just corrected.

Assembly dropped the flag again (assemble-index.mjs). bgm_pending now rides through, and a pending bed with no file on disk raises an anomaly instead of assembling a silent cut against a storyboard that promises music. Verified both branches behave differently: bgm_pending: true emits the anomaly, silent-by-design stays quiet.

Sibling adapters — there were two, not one. The review named faceless-explainer; pr-to-video carries the same file. Its own suite asserts the two are byte-identical ("intentionally identical across the reusing skills"), so patching one alone turned that test red, which is what surfaced the second copy. Both now have the absence-sentinel filter and the surviving bgm_pending, and faceless-explainer picked up the same five regression tests. All three copies verified to carry both fixes.

Miga's follow-up: the sticky-restore in finally is wrapped, with a test that a restore throwing on a broken page still lets the real capture error propagate rather than masking it.

Not touched, still follow-ups: assemble-index.mjs discarding the transitions.mjs inject block on re-assembly (needs a call on whether assemble preserves an injected block or inject becomes re-appliable), and SFX cues having no intra-frame offset.

Validation at this head: vitest run src/capture 95 pass (5 new) · product-launch audio 13 pass · faceless-explainer audio 10 pass (5 new, incl. the byte-identity contract) · bun run lint:skills · oxlint/oxfmt clean · tsc --noEmit clean.

[WaterrrForever]

Correction on my previous comment — I'd been working from a summary of this review, not the body. Re-read it, and two of the three blockers were addressed in spirit but not to the letter. Both now done in 6b93077.

Blocker 3 — I probed before neutralisation, you asked for after. 194fb69 moved the measurement off the caller's stale value and into the function, but took it before forcing fixed/sticky to static. You flagged exactly this and you're right: dropping those elements back into flow grows the document, so the probe could still read under the cap on a page that is over it once neutralised. The probe now runs after neutralisation and before the shot, inside the same try so restoration still fires on the early return. Added the case you specified — initial height under the cap, final height over it — asserting no screenshot, no file written, and the page still handed back unmodified.

Blocker 2 — I warned where you asked me to refuse. You're right that an anomaly in a list isn't enforcement, and your reasoning is the operative part: assemble gets re-run on Step 6 rework long after the audio step's warning scrolled past. assemble-index.mjs now dies on bgm_pending && !bgm, with --allow-pending-bgm as the deliberate escape for previewing mid-generate. Pinned with three tests in a new assemble-index.test.mjs: refusal writes no index.html, the escape assembles and says so, and a film that is silent by design still assembles untouched — the distinction the flag exists to make.

Blocker 1 was already done in 194fb69, and went one further than the review: pr-to-video carries the same adapter and had both bugs too. Its own suite asserts byte-identity with faceless-explainer ("intentionally identical across the reusing skills"), so patching one alone turned that test red — which is what surfaced the third copy. All three now carry both fixes.

Validation at this head: vitest run src/capture 96 pass (6 new) · product-launch audio 13 pass · assemble-index 3 pass (new file) · faceless-explainer audio 10 pass · bun run lint:skills · oxlint/oxfmt clean · tsc --noEmit clean.

Ready for another look.

[miga-heygen]

Re-review after v2 rework.

Significant improvements to the plate capture and bgm_pending flow. Three categories of change:

---

Plate: self-measuring height + PNG clipping guard + defensive finally

Three upgrades since v1, each addressing a distinct failure mode:

1. Self-measuring height. captureFullPagePlate no longer takes scrollHeight as a parameter — it probes Math.max(body.scrollHeight, documentElement.scrollHeight) itself, AFTER sticky/fixed neutralization. Both the scroll traversal (lazy loading) and neutralization (elements returning to flow) grow the document, so a pre-scroll measurement is structurally too low on the exact pages the guard exists for. ✓

2. pngHeight — post-capture clipping guard. Reads the actual PNG IHDR chunk (bytes 20-23, big-endian uint32) and discards the plate if Chrome produced something taller than MAX_PLATE_HEIGHT_PX. The capture itself can trigger another round of lazy loading, so even the self-measured height can be wrong. Belt AND suspenders. pngHeight correctly returns null for non-PNG input and short buffers. The >>> 0 unsigned coercion is correct for heights up to 2^32. ✓

3. Defensive try/catch in finally. The restoration page.evaluate is now wrapped in try/catch so a browser crash during cleanup doesn't mask the original error. This was my suggestion from the v1 review — glad to see it implemented. ✓

New tests cover all three:

• "measures the height itself, after lazy content has grown the page" — docHeight 20000 → null, screenshot never called. ✓
• "discards a plate Chrome clipped, even when the measurement passed" — docHeight 16000 but plateHeight over cap → null, file not written. ✓
• "survives a restore that throws — the real error is what propagates" — screenshot throws "capture failed", restoration throws "page crashed", caller sees "capture failed" (not "page crashed"). ✓
• "Magi's case" — post-neutralization height exceeds cap (sticky header returning to flow grows the page), guard catches it, restoration still runs. ✓
• pngHeight tests — reads IHDR correctly, returns null for non-PNG. ✓

---

bgm_pending now flows to assemble-index.mjs (new)

This wasn't in v1 and it's the right follow-through. fetch-sfx warns, but assemble-index is the step that actually builds the film — and it's often re-run during Step 6 rework, long after the audio warning scrolled past. The hard gate is the correct enforcement point.

• assemble-index.mjs reads bgm_pending from audio_meta.json. ✓
• Default: refuses to build (die()) when bgm_pending && !bgm — prevents silently shipping a film the storyboard says has music. ✓
• --allow-pending-bgm: escape hatch for previewing mid-generate, logs an anomaly. ✓
• bgm: null without bgm_pending (silent by design) still assembles — the flag is what distinguishes the two states. ✓

3 new tests (assemble-index.test.mjs): refuses+no-index, allow-pending assembles+reports, silent-by-design assembles cleanly. ✓

---

Audio fixes spread to faceless-explainer and pr-to-video

The SFX_NONE filtering and bgm_pending preservation are now in all three skill audio.mjs files. These skills share the same toProductLaunchMeta and runFetchSfx code — applying the fix only to product-launch-video would have left the same bug in the sibling skills. ✓

The faceless-explainer copy also gets the duplicate test suite. pr-to-video gets the code fix but not its own tests (it shares the same audio.mjs structure — the test coverage from the other two skills is sufficient).

---

Previously reviewed (unchanged)

• toStandaloneSvg — SVG namespace fix. ✓
• Skill wording (SKILL.md, frame-worker.md). ✓

---

CI

50 pass, 1 pending (Windows tests). Confirm before merge.

---

Verdict

Approve. The v2 rework is a substantial improvement. The plate capture is now triple-guarded (self-measured height, PNG IHDR verification, defensive finally), and bgm_pending flows all the way through to the assembly hard gate — which is the right enforcement point for preventing silent films. Good follow-through on spreading the audio fixes to all three skills.

-- Miga

[jrusso1020]

Re-review of the delta since 37961e36c (two commits, seven files). Not stamping: @miguel-heygen's CHANGES_REQUESTED is live and those were its blockers to clear, not mine to approve over.

All three are genuinely resolved, and two are closed harder than they were raised.

1. Shared audio contract — resolved, and wider than either of us scoped it. toProductLaunchMeta exists in exactly three places, and all three now carry the flag and the sentinel set. The third, skills/pr-to-video/scripts/audio.mjs, wasn't named in my finding or in the blocker; finding it was the right instinct rather than fixing only the file that got cited.

2. Assembly boundary — resolved, and stronger than requested. The ask was refuse-or-enforce; assemble-index.mjs:591 refuses by default via die, with --allow-pending-bgm as an explicit escape. The test that matters most is the third one — a film silent by design still assembles untouched. That's what proves the flag restored a distinction rather than just adding a gate.

3. Plate height guard — resolved, plus a failure mode neither of us raised. The parameter is gone and docHeight is measured in-function after neutralisation. Beyond that, pngHeight reads the IHDR chunk to check what Chrome actually produced, because the capture itself can trigger another round of lazy loading — a residual window that survived the re-measure fix. Byte offsets check out (8-byte signature, 4 length, 4 type at 12-15 = IHDR, height big-endian at 20-23, >>> 0 for the high-bit case). The requested test is at screenshotCapture.test.ts, mocking 9000 pre-neutralisation and 20000 after, asserting screenshot is never called and the page is still handed back restored.

CI at 6b9307786f: fully green now. All six workflows conclude success and there are zero non-success check-runs, Windows included — it was still in flight a few minutes ago, so that's a change since the last comment on this PR.

important — the producer fix reached all three skills, the consumer fix reached one

This is blocker 2 reproduced in the two skills that just received blocker 1's fix:

skill	audio.mjs emits bgm_pending	assemble-index.mjs reads it
product-launch-video	yes	yes
faceless-explainer	yes	no
pr-to-video	yes	no

• skills/faceless-explainer/scripts/assemble-index.mjs:352
• skills/pr-to-video/scripts/assemble-index.mjs:358

Both still rebuild the audio object as { bgm: parsed.bgm ?? null, voices, sfx }, dropping the flag their own audio step now writes. So those two workflows will do exactly what this PR set out to stop: read an audio_meta.json that says bgm_pending: true and assemble the silent film without a word.

Not a regression — before this change they didn't emit the flag either, so the outcome is unchanged rather than newly broken, which is why I'm marking it important rather than asserting a blocker. But the reasoning that made blocker 1 a blocker (a shared contract fixed in one workflow is not fixed) applies to this one unmodified, so it's reasonable to grade it the same and I'd defer to @miguel-heygen on that. The fix is the diff that already landed in product-launch-video, applied twice.

nit — the audio fix is tested in two of the three skills

skills/pr-to-video/scripts/ has no audio.test.mjs, while the other two copies both do. The code is identical, so the risk is drift rather than a live defect: the untested copy is the one that can silently diverge later. That directory already carries captions.test.mjs and frame-contract.test.mjs, so the pattern is established there.

Verdict: COMMENT
Reasoning: All three blockers are properly closed at this head and CI is green, so this is clear to stamp from my side once the holder of the changes-request is satisfied. The remaining item is the same contract gap one level down, in two sibling assemblers, and it's cheap to close while the context is loaded.

— Rames Jusso

[miguel-heygen]

The v2 closes all three findings from my first pass, and the implementation is materially stronger:

• packages/cli/src/capture/screenshotCapture.ts:83 measures after sticky/fixed neutralization, :91 verifies the produced PNG height, and restoration is protected in finally.
• The audio-model fixes now cover all three producers.
• skills/product-launch-video/scripts/assemble-index.mjs:591 enforces bgm_pending at the point that can otherwise build a silently incomplete film, while retaining an explicit preview escape hatch.

One contract-level blocker remains:

• skills/faceless-explainer/scripts/assemble-index.mjs:348-352 and skills/pr-to-video/scripts/assemble-index.mjs:354-358 both reconstruct audio with only bgm, voices, and sfx. They drop the bgm_pending field their updated audio producers now preserve, and neither has the hard gate added to the product-launch assembler. In both skills, an audio_meta.json with bgm_pending: true therefore still assembles a silent film without warning—the exact state distinction this PR is fixing. Please carry the product-launch consumer-side handling and focused pending/silent-by-design tests into these two sibling assemblers.

I verified the exact head 6b9307786fb1f733342765065227a13bb962df95: all exact-head CI is terminal-success (Windows included), the focused capture tests pass (25), product-launch audio/assembler tests pass (16), faceless audio tests pass (10), and git diff --check is clean.

Verdict: REQUEST CHANGES

Reasoning: The original blockers are resolved, but the newly shared producer contract is still discarded by two in-scope consumers, preserving the same silent-incomplete assembly failure in those workflows.

— Magi

[WaterrrForever]

Fixed in 3a6c173 — and you're right that this one is on me: the previous commit made all three adapters emit bgm_pending while only product-launch's assembler reads it. Producer fixed in three places and consumer in one is worse than neither, since before this PR there was no flag to drop.

Both siblings now get the same three changes product-launch got — flag carried through the audio object, die on bgm_pending && !bgm, --allow-pending-bgm as the deliberate escape — and the same three tests each, including the silent-by-design case. @rames Jusso called that one out as the test worth having and it's the reason it's in all three: it proves the flag restored a distinction rather than just adding a gate.

Applied as three separate patches rather than a copy, since these assemblers have diverged — pr-to-video validates a bare <template> fragment where product-launch takes a <div> root, which its fixture now reflects.

One note on scope while auditing this: there is a fourth copy of this assembler in music-to-video, which neither review named. It is deliberately untouched — that skill has no audio producer and its assembler reads { voices: [] } with no bgm path at all, so the flag can never reach it. Flagging it so "three of four" doesn't read as a miss.

Producer and consumer are now aligned across every skill that can see the flag:

skill	emits	assembler reads	gate tests
product-launch-video	✓	✓	3 pass
faceless-explainer	✓	✓	3 pass
pr-to-video	✓	✓	3 pass
music-to-video	n/a — no bgm path	n/a	n/a

Validation at this head: three assemble-index suites 3 pass each · product-launch audio 13 pass · faceless-explainer audio 10 pass · vitest run src/capture 96 pass · bun run lint:skills · oxlint/oxfmt clean · tsc --noEmit clean.

[miga-heygen]

Re-review v3 — assembly gate spread to all skills.

The only change since v2: the bgm_pending assembly gate (assemble-index.mjs + assemble-index.test.mjs) is now applied to faceless-explainer and pr-to-video too. v2 had already spread the audio.mjs fixes (SFX_NONE + bgm_pending in toProductLaunchMeta + fetch-sfx warning) to all three skills, but the assembly hard gate was only in product-launch-video. This closes the gap.

Same code, same tests, same behavior across all three skills. The pr-to-video test fixture correctly uses <template> wrapper per its frame contract. ✓

Approve. No new concerns — this is pure completeness.

-- Miga

[jrusso1020]

Delta since 6b9307786f is one commit, five files, and it closes @miguel-heygen's remaining blocker exactly as specified: the consumer-side handling and the pending/silent-by-design tests are now in both sibling assemblers.

The contract is uniform across all three skills at 3a6c173e7d:

skill	audio.mjs emits	assembler carries	hard gate	escape flag	assembler tests
product-launch-video	yes	yes	yes	yes	yes
faceless-explainer	yes	yes	yes	yes	yes
pr-to-video	yes	yes	yes	yes	yes

Grepping every assemble-index.mjs under skills/ for the old three-key reconstruction now returns nothing, so there is no fourth consumer left holding the previous shape. Both new gates are faithful copies of the reviewed one rather than reinterpretations — same refusal, same --allow-pending-bgm escape, same anomaly line.

One thing to check before stamping, not a finding: CI is not terminal at this head yet. Windows render verification is still in progress (Render on windows-latest, Tests on windows-latest); every other workflow and check-run has concluded success, manifest-in-sync and the skills suites included. The terminal-green verification in the previous review was against 6b9307786f, which this commit supersedes, so that result doesn't carry forward. Worth re-reading the Windows jobs at 3a6c173e7d before a stamp lands rather than inheriting the earlier reading.

nit, carried forward and still open: skills/pr-to-video/scripts/ has assemble-index.test.mjs now but still no audio.test.mjs, while the other two copies have both. The producer code there is identical to its tested siblings, so this is drift exposure rather than a live defect, and it's the one remaining asymmetry in an otherwise uniform set.

Verdict: COMMENT
Reasoning: The last contract-level blocker is resolved uniformly and verified across all three consumers. Holding at comment rather than approving because the changes-request is @miguel-heygen's to clear, and because Windows CI hasn't concluded at this head yet. No objection from me once both settle.

— Rames Jusso

[miguel-heygen]

The v3 closes my remaining contract blocker cleanly and uniformly.

• skills/faceless-explainer/scripts/assemble-index.mjs:359 preserves bgm_pending, and :444 refuses silent assembly by default while retaining the explicit preview escape.
• skills/pr-to-video/scripts/assemble-index.mjs:365 and :450 carry the identical producer-to-consumer contract into the third workflow.
• The new suites pin all three states in both siblings: pending refuses without writing index.html, --allow-pending-bgm deliberately proceeds, and silent-by-design remains unaffected (skills/faceless-explainer/scripts/assemble-index.test.mjs:43, skills/pr-to-video/scripts/assemble-index.test.mjs:46).

The contract audit now covers every assembler that can receive this flag; the remaining music-to-video assembler has no BGM producer/path and cannot receive it. This is additive to Miga and Rames's v3 reviews, which reached the same code-level result.

Verified exact head 3a6c173e7d7c21e319f6c9879210ad47db1ac6cb: all exact-head CI is terminal-success, including both Windows jobs; all three focused assembler suites pass locally (9/9); and git diff --check is clean.

Verdict: APPROVE

Reasoning: The last cross-workflow contract gap is closed with faithful consumer-side gates and focused failure/escape/silent-by-design coverage, and the exact head is fully green.

— Magi