fix(capture,audio): meet the two review asks I under-delivered on

Follow-up to 194fb69. Re-read Magi's review body rather than working from the summary,
and two of the three blockers were addressed in spirit but not to the letter.

**The plate probed before neutralisation, not after.** 194fb69 moved the measurement off
the caller's stale value and into the function, but took it before forcing fixed/sticky
elements to `static`. The review called this out specifically and is 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 happens on the early return. Added the exact
case asked for — initial height under the cap, final height over it — asserting no
screenshot is taken, no file is written, and the page is still handed back unmodified.

**Assembly warned where the review asked it to refuse.** An anomaly in a list is not
enforcement: assemble is re-run on Step 6 rework, long after the audio step's warning
scrolled past, and a warning still lets a silent film out the door over a snapshot whose own
JSON says the bed is generating. `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.

Validation: `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

Author: WaterrrForever

packages/cli/src/capture/screenshotCapture.test.ts

@@ -72,13 +72,13 @@ describe("captureFullPagePlate — the scroll shot's plate", () => {
     await captureFullPagePlate(page, dir);
 
     const scripts = evaluate.mock.calls.map((c) => String(c[0]));
-    // height probe, neutralise, restore
+    // neutralise, height probe, restore — the probe sits after neutralisation because
+    // forcing fixed/sticky to `static` puts those elements back in flow and grows the page.
     expect(scripts).toHaveLength(3);
-    expect(scripts[0]).toContain("scrollHeight");
-    // Neutralise before the shot — a fixed header would otherwise bake in mid-plate.
-    expect(scripts[1]).toContain("'fixed'");
-    expect(scripts[1]).toContain("'sticky'");
-    expect(scripts[1]).toContain("data-hf-plate-position");
+    expect(scripts[0]).toContain("'fixed'");
+    expect(scripts[0]).toContain("'sticky'");
+    expect(scripts[0]).toContain("data-hf-plate-position");
+    expect(scripts[1]).toContain("scrollHeight");
     // Then hand the page back unchanged: the caller keeps reading the DOM after this.
     expect(scripts[2]).toContain("removeAttribute");
     expect(scripts[2]).toContain("data-hf-plate-position");
@@ -155,3 +155,30 @@ describe("pngHeight", () => {
     expect(pngHeight(Buffer.alloc(4))).toBeNull();
   });
 });
+
+describe("captureFullPagePlate — the guard sees the post-neutralisation page (Magi's case)", () => {
+  it("skips when the initial height is under the cap but the final height is over it", async () => {
+    const dir = mkdtempSync(join(tmpdir(), "hf-plate-"));
+    // Pre-traversal the page measured 9000. Lazy content and un-fixing the sticky header push
+    // it over the cap by the time the plate would be shot. Probing before either step would
+    // have passed the guard and emitted a clipped plate.
+    let neutralised = false;
+    const evaluate = vi.fn(async (script?: unknown) => {
+      const src = String(script);
+      if (src.includes("'sticky'")) {
+        neutralised = true;
+        return undefined;
+      }
+      if (src.includes("scrollHeight")) return neutralised ? 20000 : 9000;
+      return undefined;
+    });
+    const screenshot = vi.fn(async (_opts?: unknown) => pngBuffer(20000));
+    const page = { evaluate, screenshot } as unknown as Page;
+
+    expect(await captureFullPagePlate(page, dir)).toBeNull();
+    expect(screenshot).not.toHaveBeenCalled();
+    expect(existsSync(join(dir, "full-page.png"))).toBe(false);
+    // Bailing out early must still hand the page back unmodified.
+    expect(String(evaluate.mock.calls.at(-1)?.[0])).toContain("removeAttribute");
+  });
+});

packages/cli/src/capture/screenshotCapture.ts

@@ -60,15 +60,6 @@ export async function captureFullPagePlate(
   page: Page,
   screenshotsDir: string,
 ): Promise<string | null> {
-  // Measured here rather than taken from the caller: the plate is deliberately shot AFTER the
-  // scroll traversal, and lazy content grows the document as it loads — a height measured
-  // before scrolling reads low on exactly the long pages this guard exists for, which would
-  // let the check pass and a clipped plate through.
-  const docHeight = (await page.evaluate(
-    `Math.max(document.body.scrollHeight, document.documentElement.scrollHeight)`,
-  )) as number;
-  if (docHeight > MAX_PLATE_HEIGHT_PX) return null;
-
   // Record the inline value before overwriting so the page is handed back unchanged — the
   // caller keeps using it (asset extraction, DOM reads) after this returns.
   await page.evaluate(
@@ -81,6 +72,16 @@ export async function captureFullPagePlate(
     })`,
   );
   try {
+    // Measured here — after the caller's scroll traversal AND after neutralisation — never
+    // taken from the caller. Both steps grow the document: lazy content loads as the page is
+    // scrolled, and forcing fixed/sticky elements to `static` drops them back into flow. A
+    // height read before either one is low on exactly the long pages this guard exists for,
+    // which would pass the check and let a clipped plate through.
+    const docHeight = (await page.evaluate(
+      `Math.max(document.body.scrollHeight, document.documentElement.scrollHeight)`,
+    )) as number;
+    if (docHeight > MAX_PLATE_HEIGHT_PX) return null;
+
     const buffer = await page.screenshot({ type: "png", fullPage: true });
     // Confirm what Chrome produced instead of trusting the measurement: the capture itself can
     // trigger another round of lazy loading. A clipped plate is undetectable downstream — the

skills-manifest.json

@@ -62,8 +62,8 @@
       "files": 29
     },
     "product-launch-video": {
-      "hash": "4412edf071681ceb",
-      "files": 26
+      "hash": "a0f6b1f4c8131ed2",
+      "files": 27
     },
     "remotion-to-hyperframes": {
       "hash": "3a0e6c2affb9f74e",

skills/product-launch-video/scripts/assemble-index.mjs

@@ -63,6 +63,9 @@ const flag = (name, def) => {
   const i = argv.indexOf(`--${name}`);
   return i >= 0 && i + 1 < argv.length ? argv[i + 1] : def;
 };
+// Deliberate escape from the bgm_pending refusal below — for previewing while a detached
+// generate is still running. Off by default so a silent film can't ship by accident.
+const allowPendingBgm = argv.includes("--allow-pending-bgm");
 function die(msg) {
   console.error(`✗ assemble-index.mjs: ${msg}`);
   process.exit(1);
@@ -586,12 +589,19 @@ if (audio.bgm?.path) {
     anomalies.push(`bgm ${audio.bgm.path} not on disk — skipped`);
   }
 } else if (audio.bgm_pending) {
-  // The distinction the flag exists to make: this film is not silent by design, its bed just
-  // has not finished generating. Assembling now ships a silent cut against a storyboard that
-  // promises music, so say it here rather than let the build read as complete.
+  // The distinction the flag exists to make. A warning is not enough here: assemble is re-run
+  // on Step 6 rework, long after the audio step's own warning scrolled past, and it would
+  // happily build a silent film from a snapshot whose JSON says the bed is still generating.
+  // Refuse by default; --allow-pending-bgm is the deliberate escape for previewing mid-generate.
+  if (!allowPendingBgm) {
+    die(
+      "audio_meta.json says bgm_pending — the music bed is still generating and is NOT in this " +
+        "assembly. Wait for the track, re-run the audio step, then assemble again. To assemble a " +
+        "deliberately silent preview anyway, pass --allow-pending-bgm.",
+    );
+  }
   anomalies.push(
-    "bgm is still generating (bgm_pending) — this assembly has NO music bed. Re-run the audio " +
-      "step and assemble again once the track lands, or the film ships silent.",
+    "bgm still generating (bgm_pending) — assembled without a bed per --allow-pending-bgm",
   );
 }
 

skills/product-launch-video/scripts/assemble-index.test.mjs

@@ -0,0 +1,72 @@
+import assert from "node:assert/strict";
+import { existsSync, mkdirSync, mkdtempSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { spawnSync } from "node:child_process";
+import test from "node:test";
+
+const assembleScript = new URL("./assemble-index.mjs", import.meta.url).pathname;
+
+// ── bgm_pending at the assembly boundary ─────────────────────────────────────
+// Regression: the flag survived into audio_meta.json but assemble rebuilt its audio object
+// from three named keys and dropped it, so the step that actually builds the film could not
+// tell "not ready yet" from "silent by design" and would ship the silent one.
+
+function assembleWith({ audioMeta, extraArgs = [] }) {
+  const dir = mkdtempSync(join(tmpdir(), "product-launch-assemble-"));
+  writeFileSync(
+    join(dir, "STORYBOARD.md"),
+    "---\nformat: 1920x1080\nmessage: T\n---\n\n## Frame 1 — A\n- duration: 3s\n- src: compositions/frames/01-a.html\n",
+  );
+  mkdirSync(join(dir, "compositions", "frames"), { recursive: true });
+  writeFileSync(
+    join(dir, "compositions", "frames", "01-a.html"),
+    '<div data-composition-id="01-a" data-width="1920" data-height="1080">' +
+      '<section class="clip" data-start="0" data-duration="3"></section></div>',
+  );
+  if (audioMeta) writeFileSync(join(dir, "audio_meta.json"), JSON.stringify(audioMeta));
+  const r = spawnSync(
+    process.execPath,
+    [
+      assembleScript,
+      "--storyboard",
+      join(dir, "STORYBOARD.md"),
+      "--hyperframes",
+      dir,
+      ...extraArgs,
+    ],
+    { encoding: "utf8" },
+  );
+  return { dir, r };
+}
+
+test("assemble REFUSES while bgm_pending and no bed on disk", () => {
+  const { dir, r } = assembleWith({
+    audioMeta: { bgm: null, bgm_pending: true, voices: [], sfx: [] },
+  });
+
+  assert.notEqual(r.status, 0, "should not assemble a silent film over a pending bed");
+  assert.match(r.stderr, /bgm_pending/);
+  // Refusing means producing nothing, not a half-built index.
+  assert.equal(existsSync(join(dir, "index.html")), false);
+});
+
+test("--allow-pending-bgm assembles anyway, and says so", () => {
+  const { dir, r } = assembleWith({
+    audioMeta: { bgm: null, bgm_pending: true, voices: [], sfx: [] },
+    extraArgs: ["--allow-pending-bgm"],
+  });
+
+  assert.equal(r.status, 0, r.stderr);
+  assert.equal(existsSync(join(dir, "index.html")), true);
+  assert.match(r.stdout + r.stderr, /pending/i);
+});
+
+test("a film that is silent BY DESIGN still assembles untouched", () => {
+  // The whole point of carrying the flag: this case must stay distinguishable from the above.
+  const { dir, r } = assembleWith({ audioMeta: { bgm: null, voices: [], sfx: [] } });
+
+  assert.equal(r.status, 0, r.stderr);
+  assert.equal(existsSync(join(dir, "index.html")), true);
+  assert.doesNotMatch(r.stderr, /bgm_pending/);
+});