From 7085f601fad1eb55bf30b3078c11e402b82f19f2 Mon Sep 17 00:00:00 2001 From: Miao Yang Date: Thu, 30 Jul 2026 15:51:43 +0800 Subject: [PATCH 1/6] fix(capture,audio): three defects found running product-launch-video end to end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while running the full product-launch-video workflow twice against a real site (linear.app) to verify PRs #2880/#2881/#2882. All three are independent of those PRs. **Scraped SVGs were unusable as files.** `assetDownloader` wrote an inline ``'s `outerHTML` straight to `assets/svgs/*.svg`. An inline SVG inherits its namespace from the HTML parser, so `outerHTML` omits `xmlns` — valid pasted back into HTML, but not a standalone document, and `` renders a broken-image icon. That is exactly how these assets get consumed. `toStandaloneSvg` now 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. **`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 engine as a real cue that could not resolve. The absence spellings are part of the storyboard vocabulary; drop them. **`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. `toProductLaunchMeta` returned only `{bgm, voices, sfx}`, so "not ready yet" became indistinguishable from "silent by design" — and because `fetch-sfx` rewrites `audio_meta.json` from the sidecar, a still-generating bed was snapshotted away with nothing to signal it. The flag now survives, and `fetch-sfx` warns when it snapshots a pending bed instead of leaving a silent film that the storyboard claims has music. Not included, deliberately: `assemble-index.mjs` rewrites `index.html` wholesale and so discards the block `transitions.mjs inject` wrote, meaning any Step 6 rework silently loses transitions. Fixing that 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, so it deserves its own change. Validation: `node --test skills/product-launch-video/scripts/audio.test.mjs` (13 pass, 5 new) · `vitest run src/capture` (85 pass, 5 new) · `bun run lint:skills` · oxlint/oxfmt clean · `tsc --noEmit` clean --- .../cli/src/capture/assetDownloader.test.ts | 38 ++++++++- packages/cli/src/capture/assetDownloader.ts | 32 ++++++- skills-manifest.json | 2 +- skills/product-launch-video/scripts/audio.mjs | 23 ++++- .../scripts/audio.test.mjs | 85 +++++++++++++++++++ 5 files changed, 174 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/capture/assetDownloader.test.ts b/packages/cli/src/capture/assetDownloader.test.ts index 9209e4a685..fa9da1c65d 100644 --- a/packages/cli/src/capture/assetDownloader.test.ts +++ b/packages/cli/src/capture/assetDownloader.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { isPrivateUrl, safeFetch } from "./assetDownloader.js"; +import { isPrivateUrl, safeFetch, toStandaloneSvg } from "./assetDownloader.js"; describe("isPrivateUrl — SSRF denylist (security: F-003)", () => { it("blocks loopback, private, and metadata IPv4", () => { @@ -91,3 +91,39 @@ describe("safeFetch — re-validates the denylist on every redirect hop (securit expect(fetchMock).not.toHaveBeenCalled(); }); }); + +describe("toStandaloneSvg — scraped inline SVGs must survive as .svg files", () => { + it("adds the SVG namespace that outerHTML omits for inline SVG", () => { + const inline = ''; + const out = toStandaloneSvg(inline); + expect(out).toContain('xmlns="http://www.w3.org/2000/svg"'); + // Nothing else may change — the path geometry is the brand mark. + expect(out).toContain(''); + expect(out.endsWith("")).toBe(true); + }); + + it("leaves an SVG that already declares xmlns untouched", () => { + const already = ''; + expect(toStandaloneSvg(already)).toBe(already); + }); + + it("declares xmlns:xlink only when an xlink: attribute is actually used", () => { + const withXlink = ''; + expect(toStandaloneSvg(withXlink)).toContain('xmlns:xlink="http://www.w3.org/1999/xlink"'); + const without = ''; + expect(toStandaloneSvg(without)).not.toContain("xmlns:xlink"); + }); + + it("is idempotent and preserves attributes on the root", () => { + const inline = ''; + const once = toStandaloneSvg(inline); + expect(toStandaloneSvg(once)).toBe(once); + for (const attr of ['class="logo"', 'width="120"', 'height="24"', 'fill="currentColor"']) { + expect(once).toContain(attr); + } + }); + + it("returns non-SVG input unchanged rather than corrupting it", () => { + expect(toStandaloneSvg("
not an svg
")).toBe("
not an svg
"); + }); +}); diff --git a/packages/cli/src/capture/assetDownloader.ts b/packages/cli/src/capture/assetDownloader.ts index a3f13bb378..67b2c0f750 100644 --- a/packages/cli/src/capture/assetDownloader.ts +++ b/packages/cli/src/capture/assetDownloader.ts @@ -17,6 +17,32 @@ function svgContentHashSlug(svgSource: string | Buffer, isLogo: boolean): string return isLogo ? `logo-${hash}` : `svg-${hash}`; } +/** + * Make a scraped inline `` usable as a standalone `.svg` file. + * + * An inline SVG in an HTML document inherits the SVG namespace from the parser, so the DOM's + * `outerHTML` does not serialize `xmlns`. That string is fine pasted back into HTML but is NOT + * a valid standalone document: `` renders a broken-image icon, which + * is how these assets are actually consumed downstream. Declare the namespaces on the way to disk. + * + * `xlink:href` is deprecated but still emitted by plenty of sites; an undeclared `xlink:` prefix + * is a parse error in a standalone document, so declare that too — but only when it is used. + */ +export function toStandaloneSvg(outerHTML: string): string { + const open = outerHTML.match(/]*>/i); + if (!open) return outerHTML; + const original = open[0]; + let tag = original; + const add: string[] = []; + if (!/\sxmlns\s*=/i.test(tag)) add.push('xmlns="http://www.w3.org/2000/svg"'); + if (/\sxlink:[a-z-]+\s*=/i.test(outerHTML) && !/\sxmlns:xlink\s*=/i.test(tag)) { + add.push('xmlns:xlink="http://www.w3.org/1999/xlink"'); + } + if (!add.length) return outerHTML; + tag = tag.replace(/^ ({ frame: Number(s.id), file: s.file, @@ -110,7 +115,7 @@ function toProductLaunchMeta(neutral) { duration_s: s.duration_s ?? 1, volume: s.volume ?? 0.35, })); - return { bgm, voices, sfx }; + return { bgm, bgm_pending: bgmPending, voices, sfx }; } // ── generate (TTS + BGM) ──────────────────────────────────────────────────── @@ -192,12 +197,16 @@ function runFetchSfx(argv) { const manifest = parseStoryboard(readFileSync(storyboardPath, "utf8")); // Per-frame `sfx:` cues (comma-separated) → engine lines carrying only sfx. + // `filter(Boolean)` alone is not enough: a storyboard that spells "no SFX here" as + // `sfx: none` used to reach the engine as a cue literally NAMED "none", which then failed + // to resolve. The absence sentinels are part of the storyboard vocabulary, so drop them. + const SFX_NONE = new Set(["none", "no", "n/a", "na", "skip", "-", "—", "–"]); const lines = []; for (const f of manifest.frames) { const names = (f.extra?.sfx ?? "") .split(",") .map((s) => s.trim()) - .filter(Boolean); + .filter((s) => s && !SFX_NONE.has(s.toLowerCase())); if (names.length && f.number != null) lines.push({ id: pad2(f.number), sfx: names }); } @@ -211,6 +220,16 @@ function runFetchSfx(argv) { const meta = toProductLaunchMeta(JSON.parse(readFileSync(neutral, "utf8"))); writeFileSync(outPath, JSON.stringify(meta, null, 2)); console.log(`✓ audio fetch-sfx: ${meta.sfx.length} SFX cue(s) → ${outPath}`); + // This pass rewrites audio_meta.json from the neutral sidecar. If a detached BGM generate is + // still running, the bed it eventually writes is NOT folded back in — the snapshot we just + // took has no music. Say so instead of leaving a silent film that the storyboard claims has a + // bed (observed live: the caller had to notice on its own and rebuild the entry). + if (meta.bgm_pending && !meta.bgm) { + console.warn( + "⚠ audio fetch-sfx: a detached BGM generate is still pending, so this snapshot has no bed. " + + "Re-run `fetch-sfx` (or re-point audio_meta.json at the track) once it lands, before assembling.", + ); + } } // ── sync-durations (local; rewrites STORYBOARD.md) ──────────────────────────── diff --git a/skills/product-launch-video/scripts/audio.test.mjs b/skills/product-launch-video/scripts/audio.test.mjs index 2921d7a5f0..47f548d666 100644 --- a/skills/product-launch-video/scripts/audio.test.mjs +++ b/skills/product-launch-video/scripts/audio.test.mjs @@ -131,3 +131,88 @@ test("a storyboard music mood still retrieves BGM (marker is exact, not fuzzy)", assert.equal(request.bgm.mode, "retrieve"); assert.equal(request.bgm.query, "upbeat synthwave with heavy drums"); }); + +// ── fetch-sfx ──────────────────────────────────────────────────────────────── +// Regressions found while running the full product-launch workflow end to end +// (linear.app site showcase, 2026-07-30). + +/** Runs the fetch-sfx subcommand. `neutralOut` is what the stub engine writes to --out. */ +function runFetchSfx({ storyboard, neutralOut = { voices: [], bgm: null, sfx: [] } }) { + const dir = mkdtempSync(join(tmpdir(), "product-launch-sfx-")); + const engine = join(dir, "engine.mjs"); + writeFileSync(join(dir, "STORYBOARD.md"), storyboard); + writeFileSync( + engine, + `import { readFileSync, writeFileSync } from "node:fs"; +const argv = process.argv.slice(2); +const flag = (name) => argv[argv.indexOf(name) + 1]; +const request = JSON.parse(readFileSync(flag("--request"), "utf8")); +writeFileSync(new URL("request.json", import.meta.url), JSON.stringify(request)); +writeFileSync(flag("--out"), ${JSON.stringify(JSON.stringify(neutralOut))}); +`, + ); + const result = spawnSync( + process.execPath, + [script, "fetch-sfx", "--hyperframes", dir, "--storyboard", join(dir, "STORYBOARD.md")], + { encoding: "utf8", env: { ...process.env, HF_MEDIA_ENGINE: engine } }, + ); + return { dir, result }; +} + +const FRAME_WITH_SFX = (sfx) => + `---\nmessage: Test\n---\n\n## Frame 1 — Hook\n- duration: 3s\n- sfx: ${sfx}\n`; + +test("fetch-sfx: `sfx: none` is an absence marker, not a cue named none", () => { + const { dir, result } = runFetchSfx({ storyboard: FRAME_WITH_SFX("none") }); + + assert.equal(result.status, 0, result.stderr); + const request = JSON.parse(readFileSync(join(dir, "request.json"), "utf8")); + // Used to reach the engine as { sfx: ["none"] } — a cue that cannot resolve. + assert.deepEqual(request.lines, []); +}); + +test("fetch-sfx: the other absence spellings are markers too", () => { + for (const spelling of ["None", "n/a", "NA", "skip", "-", "—"]) { + const { dir, result } = runFetchSfx({ storyboard: FRAME_WITH_SFX(spelling) }); + assert.equal(result.status, 0, result.stderr); + const request = JSON.parse(readFileSync(join(dir, "request.json"), "utf8")); + assert.deepEqual(request.lines, [], `spelling: ${spelling}`); + } +}); + +test("fetch-sfx: a real cue still reaches the engine, and mixed lists drop only the marker", () => { + const { dir, result } = runFetchSfx({ storyboard: FRAME_WITH_SFX("whoosh, none, click") }); + + assert.equal(result.status, 0, result.stderr); + const request = JSON.parse(readFileSync(join(dir, "request.json"), "utf8")); + assert.deepEqual(request.lines, [{ id: "01", sfx: ["whoosh", "click"] }]); +}); + +test("fetch-sfx: carries bgm_pending through and warns that the snapshot has no bed", () => { + const { dir, result } = runFetchSfx({ + storyboard: FRAME_WITH_SFX("whoosh"), + // A detached Lyria/MusicGen generate that has not landed yet. + neutralOut: { voices: [], bgm: null, bgm_pending: true, sfx: [] }, + }); + + assert.equal(result.status, 0, result.stderr); + const meta = JSON.parse(readFileSync(join(dir, "audio_meta.json"), "utf8")); + // The flag used to be dropped in the neutral → PL translation, making "not ready yet" + // indistinguishable from "silent by design". + assert.equal(meta.bgm_pending, true); + assert.equal(meta.bgm, null); + assert.match(result.stderr + result.stdout, /pending/i); +}); + +test("fetch-sfx: a resolved bed reports bgm_pending false and no warning", () => { + const { dir, result } = runFetchSfx({ + storyboard: FRAME_WITH_SFX("whoosh"), + neutralOut: { voices: [], bgm: { path: "assets/bgm/track.mp3", volume: 0.12 }, sfx: [] }, + }); + + assert.equal(result.status, 0, result.stderr); + const meta = JSON.parse(readFileSync(join(dir, "audio_meta.json"), "utf8")); + assert.equal(meta.bgm_pending, false); + assert.equal(meta.bgm.path, "assets/bgm/track.mp3"); + assert.doesNotMatch(result.stderr, /pending/i); +}); From 33dda0480df16abc4aadfda3b9a4cba7936145c3 Mon Sep 17 00:00:00 2001 From: Miao Yang Date: Thu, 30 Jul 2026 16:14:04 +0800 Subject: [PATCH 2/6] feat(capture): re-add the full-page plate a scroll shot needs, at 1x MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `product-launch-video` tells a scroll shot to animate a viewport over a full-page capture. No such file existed: capture emits 15 viewport-sized scroll-position tiles, and a plate is not substitutable by tiles — a viewport travelling down one continuous image is the whole point. An earlier `full-page.png` was dropped in 62b55171e because 1/8 agents read it and the contact sheet covered the same ground. That measured it as a *comprehension* artifact, on an eval where nothing was building scroll shots. The scroll shot is a different consumer, so this brings the plate back — but not as it was, because two things have to hold for it to be 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, which is a good reason for an agent to look once and never again. - **Sticky chrome neutralised.** `fullPage` bakes a fixed header in at one position, freezing a nav across the middle of the plate. The viewport tiles keep sticky on purpose (natural browsing state); the plate cannot. Positions are recorded and restored in a `finally`, so the extraction passes that run afterwards see an unmodified DOM. **1x, deliberately.** 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). At 1x a 1920-wide plate is pixel-exact for a 1920x1080 viewport. A frame that needs 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. Validation: `vitest run src/capture` — 90 pass (5 new) · oxlint/oxfmt clean · `tsc --noEmit` clean --- .../cli/src/capture/screenshotCapture.test.ts | 85 +++++++++++++++++++ packages/cli/src/capture/screenshotCapture.ts | 64 +++++++++++++- 2 files changed, 148 insertions(+), 1 deletion(-) create mode 100644 packages/cli/src/capture/screenshotCapture.test.ts diff --git a/packages/cli/src/capture/screenshotCapture.test.ts b/packages/cli/src/capture/screenshotCapture.test.ts new file mode 100644 index 0000000000..94e3a691d8 --- /dev/null +++ b/packages/cli/src/capture/screenshotCapture.test.ts @@ -0,0 +1,85 @@ +import { describe, expect, it, vi } from "vitest"; +import { existsSync, mkdtempSync, readFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { Page } from "puppeteer-core"; +import { captureFullPagePlate, MAX_PLATE_HEIGHT_PX } from "./screenshotCapture.js"; + +// The mocks declare their parameters so `mock.calls[i][0]` is a real slot — a zero-arg +// vi.fn() types its call tuple as [] and indexing it is a compile error. +function fakePage(overrides: Record = {}) { + const evaluate = vi.fn(async (_script?: unknown) => undefined); + const screenshot = vi.fn(async (_opts?: unknown) => Buffer.from("PNG-BYTES")); + return { page: { evaluate, screenshot, ...overrides } as unknown as Page, evaluate, screenshot }; +} + +describe("captureFullPagePlate — the scroll shot's plate", () => { + it("writes one full-page png and returns its relative path", async () => { + const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); + const { page, screenshot } = fakePage(); + + const out = await captureFullPagePlate(page, dir, 10962); + + expect(out).toBe("screenshots/full-page.png"); + expect(screenshot).toHaveBeenCalledWith({ type: "png", fullPage: true }); + expect(readFileSync(join(dir, "full-page.png"), "utf8")).toBe("PNG-BYTES"); + }); + + it("stays 1x: it never touches the viewport's deviceScaleFactor", async () => { + const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); + const setViewport = vi.fn(async () => undefined); + const { page } = fakePage({ setViewport }); + + await captureFullPagePlate(page, dir, 4000); + + // A 2x plate would exceed the cap on exactly the long pages that want a scroll shot. + expect(setViewport).not.toHaveBeenCalled(); + }); + + it("skips a page taller than Chrome can capture, instead of writing a clipped plate", async () => { + const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); + const { page, screenshot } = fakePage(); + + const out = await captureFullPagePlate(page, dir, MAX_PLATE_HEIGHT_PX + 1); + + expect(out).toBeNull(); + expect(screenshot).not.toHaveBeenCalled(); + expect(existsSync(join(dir, "full-page.png"))).toBe(false); + }); + + it("neutralises sticky/fixed chrome for the shot and restores it afterwards", async () => { + const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); + const { page, evaluate, screenshot } = fakePage(); + + await captureFullPagePlate(page, dir, 8000); + + const scripts = evaluate.mock.calls.map((c) => String(c[0])); + expect(scripts).toHaveLength(2); + // Neutralise first — a fixed header would otherwise bake in mid-plate. + expect(scripts[0]).toContain("'fixed'"); + expect(scripts[0]).toContain("'sticky'"); + expect(scripts[0]).toContain("data-hf-plate-position"); + // Then hand the page back unchanged: the caller keeps reading the DOM after this. + expect(scripts[1]).toContain("removeAttribute"); + expect(scripts[1]).toContain("data-hf-plate-position"); + expect(evaluate.mock.invocationCallOrder[0]).toBeLessThan( + screenshot.mock.invocationCallOrder[0]!, + ); + expect(screenshot.mock.invocationCallOrder[0]).toBeLessThan( + evaluate.mock.invocationCallOrder[1]!, + ); + }); + + it("restores the page even when the screenshot throws", async () => { + const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); + const screenshot = vi.fn(async (_opts?: unknown) => { + throw new Error("capture failed"); + }); + const { page, evaluate } = fakePage({ screenshot }); + + await expect(captureFullPagePlate(page, dir, 8000)).rejects.toThrow("capture failed"); + // A page left with every sticky element forced static would corrupt the extraction + // passes that run after this one. + expect(String(evaluate.mock.calls.at(-1)?.[0])).toContain("removeAttribute"); + }); +}); diff --git a/packages/cli/src/capture/screenshotCapture.ts b/packages/cli/src/capture/screenshotCapture.ts index 05c294991e..0ffc29778f 100644 --- a/packages/cli/src/capture/screenshotCapture.ts +++ b/packages/cli/src/capture/screenshotCapture.ts @@ -21,6 +21,62 @@ import { join } from "node:path"; * elements — screenshots show the page in its natural browsing state with * scroll-triggered animations fired. */ +/** + * Chrome caps a screenshot at 16384px per side (Skia's max texture dimension); past that the + * capture comes back clipped or fails outright. Long marketing pages do reach this. + */ +export const MAX_PLATE_HEIGHT_PX = 16384; + +/** + * One tall image of the whole document — the plate a scroll shot slides its viewport over. + * + * This is deliberately 1x. A 2x plate is what you'd want for pushing in without softening + * text, but doubling a long marketing page blows past `MAX_PLATE_HEIGHT_PX` exactly on the + * pages that most want a scroll shot; a frame that needs 2x should capture its own region + * instead. At 1x a 1920-wide plate is pixel-exact for a 1920x1080 viewport travelling down it. + * + * Two things have to be true for the plate to be usable, and both are why the earlier + * `full-page.png` was worth removing rather than keeping as-is: + * · It must be taken 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. + * · Sticky/fixed chrome has to be neutralised first. `fullPage` bakes a fixed header in at + * one position, so a nav ends up frozen across the middle of the plate. The viewport + * shots keep sticky on purpose (natural browsing state); the plate cannot. + * + * Returns the relative path, or null when the page is too tall to capture in one piece. + */ +export async function captureFullPagePlate( + page: Page, + screenshotsDir: string, + scrollHeight: number, +): Promise { + if (scrollHeight > 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( + `document.querySelectorAll('*').forEach((el) => { + const p = getComputedStyle(el).position; + if (p === 'fixed' || p === 'sticky') { + el.setAttribute('data-hf-plate-position', el.style.position || ''); + el.style.position = 'static'; + } + })`, + ); + try { + const buffer = await page.screenshot({ type: "png", fullPage: true }); + writeFileSync(join(screenshotsDir, "full-page.png"), buffer); + return "screenshots/full-page.png"; + } finally { + await page.evaluate( + `document.querySelectorAll('[data-hf-plate-position]').forEach((el) => { + el.style.position = el.getAttribute('data-hf-plate-position'); + el.removeAttribute('data-hf-plate-position'); + })`, + ); + } +} + export async function captureScrollScreenshots(page: Page, outputDir: string): Promise { const screenshotsDir = join(outputDir, "screenshots"); mkdirSync(screenshotsDir, { recursive: true }); @@ -151,7 +207,13 @@ export async function captureScrollScreenshots(page: Page, outputDir: string): P await page.evaluate(`window.scrollTo(0, 0)`); await new Promise((r) => setTimeout(r, 200)); - // full-page.png removed — 1/8 agents read it, contact sheet covers the same content + // The scroll plate, last: everything above has loaded the page and fired its reveals, which + // is the only state a full-page shot is worth taking in. (An earlier full-page.png was + // dropped because 1/8 agents read it and the contact sheet covered the same ground — that + // was about it as a *comprehension* artifact. The scroll shot is a different consumer: it + // needs one continuous plate, which no set of viewport tiles can substitute for.) + const plate = await captureFullPagePlate(page, screenshotsDir, scrollHeight); + if (plate) filePaths.push(plate); } catch { /* scroll screenshots are non-critical */ } From 37961e36c6a59a1d37db69889a8a9fbcf90af667 Mon Sep 17 00:00:00 2001 From: Miao Yang Date: Thu, 30 Jul 2026 16:37:18 +0800 Subject: [PATCH 3/6] docs(product-launch-video): point the scroll shot at the plate, make handoff fields binding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from the same end-to-end runs, now that #2880 and #2881 have landed and their sentences exist to edit. **The scroll shot pointed at an artifact that did not exist.** #2881 said "use a 2x full-page capture and animate the viewport over it". Neither half held: capture emitted no full-page image, and 2x on a long marketing page passes Chrome's 16384px screenshot cap precisely on the pages that most want a scroll shot. 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 1920x1080 tiles full-bleed. This PR's capture commit adds the 1x plate, so the sentence can now name something real: the plate, 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 of its own. **A constant field was being read as an absent one.** #2880 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 of the instruction. But downstream an omission and "there is no handoff here" are the same thing, so the field set has to be stated as binding even when constant. Same clause added to the worker's side of the contract. Validation: `bun run lint:skills` --- skills-manifest.json | 2 +- skills/product-launch-video/SKILL.md | 4 ++-- skills/product-launch-video/sub-agents/frame-worker.md | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/skills-manifest.json b/skills-manifest.json index d01c748d95..de1d76427b 100644 --- a/skills-manifest.json +++ b/skills-manifest.json @@ -62,7 +62,7 @@ "files": 29 }, "product-launch-video": { - "hash": "6f2b690d22392425", + "hash": "ead12de8df2ed55d", "files": 26 }, "remotion-to-hyperframes": { diff --git a/skills/product-launch-video/SKILL.md b/skills/product-launch-video/SKILL.md index 10f5dc6597..074a12d7ce 100644 --- a/skills/product-launch-video/SKILL.md +++ b/skills/product-launch-video/SKILL.md @@ -54,7 +54,7 @@ Classify the input and choose the path. Explicit URL -> capture it and use the s Run capture with: `npx hyperframes capture "" -o ./capture` -For a site tour or show-it-as-is brief, the captured page is the visual source of truth. Use the real screenshot instead of rebuilding the full website in HTML. If the shot needs internal movement, keep the screenshot as the base and overlay real captured assets at measured positions, or rebuild only the one component that moves. For a scroll shot, use a 2x full-page capture and animate the viewport over it. Recreate the whole page only when the user explicitly asks for a stylized interpretation or the capture is unusable. +For a site tour or show-it-as-is brief, the captured page is the visual source of truth. Use the real screenshot instead of rebuilding the full website in HTML. If the shot needs internal movement, keep the screenshot as the base and overlay real captured assets at measured positions, or rebuild only the one component that moves. For a scroll shot, animate the viewport over `capture/screenshots/full-page.png` — the 1x plate of the whole document, pixel-exact for a 1920-wide viewport travelling down it. It is absent when the page was too tall to capture in one piece; fall back to the overlapping scroll-position shots in the same directory. Pushing in past 1:1 wants its own 2x capture of that region instead, since the plate has no headroom above 1x. Recreate the whole page only when the user explicitly asks for a stylized interpretation or the capture is unusable. If `GEMINI_API_KEY`, `GOOGLE_API_KEY`, or an OpenRouter key exists, capture auto-captions assets into `capture/extracted/asset-descriptions.md`. This is not a review gate. Without a vision key, use DOM context and continue. @@ -128,7 +128,7 @@ Read `references/visual-design.md`, `../hyperframes-animation/blueprints-index.m For every visual frame, write a **time-coded shot sequence** into `STORYBOARD.md` per `visual-design.md`'s method: pick the frame's blueprint (or compose), instantiate it with THIS product's content, and pace each Scene's reveal to the voiceover so the frame develops across its full duration instead of front-loading then freezing. State layout and motion **inline** per Scene (vocabularies in `visual-design.md` and `motion-language.md`). Add one video-wide `## Video direction` block. -When an element visibly continues across a frame boundary, give both workers the same numerical handoff in `STORYBOARD.md`: add `handoff_out:` to the outgoing frame and a matching `handoff_in:` to the incoming frame. Name the element and its exact x/y position, scale, opacity, and motion direction/speed at the cut. Omit these fields for a deliberate clean cut. The goal is simple: parallel workers must not invent two different versions of the same seam. +When an element visibly continues across a frame boundary, give both workers the same numerical handoff in `STORYBOARD.md`: add `handoff_out:` to the outgoing frame and a matching `handoff_in:` to the incoming frame. Name the element and its exact x/y position, scale, opacity, and motion direction/speed at the cut — state every field even when it does not change, because a constant is `opacity: 1`, not an omission. Omit the whole block only for a deliberate clean cut. The goal is simple: parallel workers must not invent two different versions of the same seam. Do not change story, script, asset choices, `asset_candidates`, `transition_in`, or captured source material. Do not write HTML in this step. diff --git a/skills/product-launch-video/sub-agents/frame-worker.md b/skills/product-launch-video/sub-agents/frame-worker.md index 448519f3fa..fd5a34d7ad 100644 --- a/skills/product-launch-video/sub-agents/frame-worker.md +++ b/skills/product-launch-video/sub-agents/frame-worker.md @@ -18,4 +18,4 @@ Brand text comes from your frame's `scene` / narrative — never from `frame.md` ## Cross-frame handoffs -If the packet includes `handoff_in:` or `handoff_out:`, treat those values as a hard boundary contract. Start or end the named element at the exact x/y position, scale, opacity, and motion direction/speed provided. Do not restyle or reinterpret that boundary state. The neighboring frame is being built by another worker and will use the matching values. +If the packet includes `handoff_in:` or `handoff_out:`, treat those values as a hard boundary contract. Start or end the named element at the exact x/y position, scale, opacity, and motion direction/speed provided; a field the packet states as unchanged is still binding, not optional. Do not restyle or reinterpret that boundary state. The neighboring frame is being built by another worker and will use the matching values. From 194fb699579d04654f6e8cb16038862a4fd1e035 Mon Sep 17 00:00:00 2001 From: Miao Yang Date: Thu, 30 Jul 2026 17:31:31 +0800 Subject: [PATCH 4/6] fix(capture,audio): close the three contract gaps raised in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #2892 (Rames, Magi) found the fixes correct inside the changed files but incomplete at the contract level. All three hold up against source; two of the three were reachable in production, and the plate one was self-inflicted by this PR. **The plate guard checked a stale height.** `scrollHeight` was measured before the scroll traversal and handed to the guard, but the plate is deliberately shot *after* it so lazy content has loaded — and lazy loading grows the document. The guard's input therefore read low on exactly the long pages it exists for, letting the check pass and a clipped plate through, undetectable downstream because the skill only teaches the tile fallback when the file is *absent*. `captureFullPagePlate` now measures the height itself at call time, and verifies what Chrome actually produced by reading the PNG's IHDR before writing, since the capture can trigger another round of loading. Over the cap, nothing is emitted. **Assembly dropped the flag again.** `bgm_pending` survived into `audio_meta.json` but `assemble-index.mjs` rebuilt its audio object from three named keys, so at the step that actually builds the film "not ready yet" still looked like "silent by design" — this PR's own framing of the defect, one layer further down. The flag rides along now, and a pending bed with no file raises an anomaly instead of quietly assembling a silent cut against a storyboard that promises music. **The sibling adapters had both audio bugs, and there were two of them.** The review named `faceless-explainer`; `pr-to-video` carries the same file. Its own test asserts the two are byte-identical ("intentionally identical across the reusing skills"), so fixing one alone broke that test — which is what caught the second copy. Both now carry the absence-sentinel filter and the surviving `bgm_pending`, and `faceless-explainer` gets the same five regression tests. Also from review (Miga): the sticky-restore in `finally` is wrapped, so a page that broke mid-capture cannot replace the real error with a cleanup one. Validation: `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 --- .../cli/src/capture/screenshotCapture.test.ts | 118 ++++++++++++++---- packages/cli/src/capture/screenshotCapture.ts | 47 +++++-- skills-manifest.json | 6 +- skills/faceless-explainer/scripts/audio.mjs | 22 +++- .../faceless-explainer/scripts/audio.test.mjs | 85 +++++++++++++ skills/pr-to-video/scripts/audio.mjs | 22 +++- .../scripts/assemble-index.mjs | 17 ++- 7 files changed, 277 insertions(+), 40 deletions(-) diff --git a/packages/cli/src/capture/screenshotCapture.test.ts b/packages/cli/src/capture/screenshotCapture.test.ts index 94e3a691d8..d8dc14569b 100644 --- a/packages/cli/src/capture/screenshotCapture.test.ts +++ b/packages/cli/src/capture/screenshotCapture.test.ts @@ -3,34 +3,52 @@ import { existsSync, mkdtempSync, readFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { Page } from "puppeteer-core"; -import { captureFullPagePlate, MAX_PLATE_HEIGHT_PX } from "./screenshotCapture.js"; +import { captureFullPagePlate, MAX_PLATE_HEIGHT_PX, pngHeight } from "./screenshotCapture.js"; + +// A real 1920x800 PNG header, so the produced-height guard sees something valid. +function pngBuffer(height: number, width = 1920): Buffer { + const buf = Buffer.alloc(24); + Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]).copy(buf, 0); + buf.writeUInt32BE(13, 8); + buf.write("IHDR", 12, "ascii"); + buf.writeUInt32BE(width, 16); + buf.writeUInt32BE(height, 20); + return buf; +} // The mocks declare their parameters so `mock.calls[i][0]` is a real slot — a zero-arg // vi.fn() types its call tuple as [] and indexing it is a compile error. -function fakePage(overrides: Record = {}) { - const evaluate = vi.fn(async (_script?: unknown) => undefined); - const screenshot = vi.fn(async (_opts?: unknown) => Buffer.from("PNG-BYTES")); +// `docHeight` is what the in-function measurement returns; the plate reads the page height +// itself now rather than trusting a value the caller measured before scrolling. +function fakePage( + { docHeight = 8000, plateHeight = 8000 }: { docHeight?: number; plateHeight?: number } = {}, + overrides: Record = {}, +) { + const evaluate = vi.fn(async (script?: unknown) => + String(script).includes("scrollHeight") ? docHeight : undefined, + ); + const screenshot = vi.fn(async (_opts?: unknown) => pngBuffer(plateHeight)); return { page: { evaluate, screenshot, ...overrides } as unknown as Page, evaluate, screenshot }; } describe("captureFullPagePlate — the scroll shot's plate", () => { it("writes one full-page png and returns its relative path", async () => { const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); - const { page, screenshot } = fakePage(); + const { page, screenshot } = fakePage({ docHeight: 10962, plateHeight: 10962 }); - const out = await captureFullPagePlate(page, dir, 10962); + const out = await captureFullPagePlate(page, dir); expect(out).toBe("screenshots/full-page.png"); expect(screenshot).toHaveBeenCalledWith({ type: "png", fullPage: true }); - expect(readFileSync(join(dir, "full-page.png"), "utf8")).toBe("PNG-BYTES"); + expect(pngHeight(readFileSync(join(dir, "full-page.png")))).toBe(10962); }); it("stays 1x: it never touches the viewport's deviceScaleFactor", async () => { const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); const setViewport = vi.fn(async () => undefined); - const { page } = fakePage({ setViewport }); + const { page } = fakePage({}, { setViewport }); - await captureFullPagePlate(page, dir, 4000); + await captureFullPagePlate(page, dir); // A 2x plate would exceed the cap on exactly the long pages that want a scroll shot. expect(setViewport).not.toHaveBeenCalled(); @@ -38,9 +56,9 @@ describe("captureFullPagePlate — the scroll shot's plate", () => { it("skips a page taller than Chrome can capture, instead of writing a clipped plate", async () => { const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); - const { page, screenshot } = fakePage(); + const { page, screenshot } = fakePage({ docHeight: MAX_PLATE_HEIGHT_PX + 1 }); - const out = await captureFullPagePlate(page, dir, MAX_PLATE_HEIGHT_PX + 1); + const out = await captureFullPagePlate(page, dir); expect(out).toBeNull(); expect(screenshot).not.toHaveBeenCalled(); @@ -51,22 +69,24 @@ describe("captureFullPagePlate — the scroll shot's plate", () => { const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); const { page, evaluate, screenshot } = fakePage(); - await captureFullPagePlate(page, dir, 8000); + await captureFullPagePlate(page, dir); const scripts = evaluate.mock.calls.map((c) => String(c[0])); - expect(scripts).toHaveLength(2); - // Neutralise first — a fixed header would otherwise bake in mid-plate. - expect(scripts[0]).toContain("'fixed'"); - expect(scripts[0]).toContain("'sticky'"); - expect(scripts[0]).toContain("data-hf-plate-position"); - // Then hand the page back unchanged: the caller keeps reading the DOM after this. - expect(scripts[1]).toContain("removeAttribute"); + // height probe, neutralise, restore + 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(evaluate.mock.invocationCallOrder[0]).toBeLessThan( + // 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"); + expect(evaluate.mock.invocationCallOrder[1]).toBeLessThan( screenshot.mock.invocationCallOrder[0]!, ); expect(screenshot.mock.invocationCallOrder[0]).toBeLessThan( - evaluate.mock.invocationCallOrder[1]!, + evaluate.mock.invocationCallOrder[2]!, ); }); @@ -75,11 +95,63 @@ describe("captureFullPagePlate — the scroll shot's plate", () => { const screenshot = vi.fn(async (_opts?: unknown) => { throw new Error("capture failed"); }); - const { page, evaluate } = fakePage({ screenshot }); + const { page, evaluate } = fakePage({}, { screenshot }); - await expect(captureFullPagePlate(page, dir, 8000)).rejects.toThrow("capture failed"); + await expect(captureFullPagePlate(page, dir)).rejects.toThrow("capture failed"); // A page left with every sticky element forced static would corrupt the extraction // passes that run after this one. expect(String(evaluate.mock.calls.at(-1)?.[0])).toContain("removeAttribute"); }); }); + +describe("captureFullPagePlate — guards against a silently clipped plate", () => { + it("measures the height itself, after lazy content has grown the page", async () => { + const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); + // A page that measured 9000 before scrolling but is 20000 once lazy images land: the + // pre-scroll number would have passed the guard and emitted a clipped plate. + const { page, screenshot } = fakePage({ docHeight: 20000 }); + + expect(await captureFullPagePlate(page, dir)).toBeNull(); + expect(screenshot).not.toHaveBeenCalled(); + }); + + it("discards a plate Chrome clipped, even when the measurement passed", async () => { + const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); + // Measurement said 16000, but the capture itself triggered more loading and came back + // over the cap. Emitting it would be undetectable downstream. + const { page } = fakePage({ docHeight: 16000, plateHeight: MAX_PLATE_HEIGHT_PX + 500 }); + + expect(await captureFullPagePlate(page, dir)).toBeNull(); + expect(existsSync(join(dir, "full-page.png"))).toBe(false); + }); + + it("survives a restore that throws — the real error is what propagates", async () => { + const dir = mkdtempSync(join(tmpdir(), "hf-plate-")); + let call = 0; + const evaluate = vi.fn(async (script?: unknown) => { + call++; + if (String(script).includes("scrollHeight")) return 8000; + if (String(script).includes("removeAttribute")) throw new Error("page crashed"); + return undefined; + }); + const screenshot = vi.fn(async (_opts?: unknown) => { + throw new Error("capture failed"); + }); + const page = { evaluate, screenshot } as unknown as Page; + + // Without the try/catch in `finally`, "page crashed" would mask "capture failed". + await expect(captureFullPagePlate(page, dir)).rejects.toThrow("capture failed"); + expect(call).toBeGreaterThanOrEqual(3); + }); +}); + +describe("pngHeight", () => { + it("reads the height out of the IHDR chunk", () => { + expect(pngHeight(pngBuffer(10962))).toBe(10962); + }); + + it("returns null for anything that is not a PNG", () => { + expect(pngHeight(Buffer.from("not a png at all, definitely not"))).toBeNull(); + expect(pngHeight(Buffer.alloc(4))).toBeNull(); + }); +}); diff --git a/packages/cli/src/capture/screenshotCapture.ts b/packages/cli/src/capture/screenshotCapture.ts index 0ffc29778f..f574f4fd65 100644 --- a/packages/cli/src/capture/screenshotCapture.ts +++ b/packages/cli/src/capture/screenshotCapture.ts @@ -27,6 +27,17 @@ import { join } from "node:path"; */ export const MAX_PLATE_HEIGHT_PX = 16384; +/** + * Pixel height Chrome actually produced, read from the PNG's IHDR chunk: 8-byte signature, + * then 4 length + 4 type + 4 width + 4 height. Null when the buffer isn't a PNG. + */ +export function pngHeight(buf: Uint8Array): number | null { + // Byte math rather than Buffer helpers: page.screenshot() resolves to a Uint8Array. + if (buf.length < 24) return null; + if (buf[12] !== 0x49 || buf[13] !== 0x48 || buf[14] !== 0x44 || buf[15] !== 0x52) return null; // "IHDR" + return ((buf[20]! << 24) | (buf[21]! << 16) | (buf[22]! << 8) | buf[23]!) >>> 0; +} + /** * One tall image of the whole document — the plate a scroll shot slides its viewport over. * @@ -48,9 +59,15 @@ export const MAX_PLATE_HEIGHT_PX = 16384; export async function captureFullPagePlate( page: Page, screenshotsDir: string, - scrollHeight: number, ): Promise { - if (scrollHeight > MAX_PLATE_HEIGHT_PX) return 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. @@ -65,15 +82,27 @@ export async function captureFullPagePlate( ); try { 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 + // skill only teaches the tile fallback when the file is *absent* — so emit nothing rather + // than something silently wrong. + const produced = pngHeight(buffer); + if (produced != null && produced > MAX_PLATE_HEIGHT_PX) return null; writeFileSync(join(screenshotsDir, "full-page.png"), buffer); return "screenshots/full-page.png"; } finally { - await page.evaluate( - `document.querySelectorAll('[data-hf-plate-position]').forEach((el) => { - el.style.position = el.getAttribute('data-hf-plate-position'); - el.removeAttribute('data-hf-plate-position'); - })`, - ); + // A page that broke mid-capture will fail this too; letting that escape would replace the + // real error with a cleanup one. Nothing to restore if the page is already gone. + try { + await page.evaluate( + `document.querySelectorAll('[data-hf-plate-position]').forEach((el) => { + el.style.position = el.getAttribute('data-hf-plate-position'); + el.removeAttribute('data-hf-plate-position'); + })`, + ); + } catch { + /* page unusable — the restore is moot */ + } } } @@ -212,7 +241,7 @@ export async function captureScrollScreenshots(page: Page, outputDir: string): P // dropped because 1/8 agents read it and the contact sheet covered the same ground — that // was about it as a *comprehension* artifact. The scroll shot is a different consumer: it // needs one continuous plate, which no set of viewport tiles can substitute for.) - const plate = await captureFullPagePlate(page, screenshotsDir, scrollHeight); + const plate = await captureFullPagePlate(page, screenshotsDir); if (plate) filePaths.push(plate); } catch { /* scroll screenshots are non-critical */ diff --git a/skills-manifest.json b/skills-manifest.json index de1d76427b..8284f6962d 100644 --- a/skills-manifest.json +++ b/skills-manifest.json @@ -6,7 +6,7 @@ "files": 140 }, "faceless-explainer": { - "hash": "b772a9b6c8118c2c", + "hash": "b458fdb62c5e1402", "files": 22 }, "figma": { @@ -58,11 +58,11 @@ "files": 132 }, "pr-to-video": { - "hash": "44a9877e7ea1289e", + "hash": "01dc26f444bc20cd", "files": 29 }, "product-launch-video": { - "hash": "ead12de8df2ed55d", + "hash": "4412edf071681ceb", "files": 26 }, "remotion-to-hyperframes": { diff --git a/skills/faceless-explainer/scripts/audio.mjs b/skills/faceless-explainer/scripts/audio.mjs index 3bae9e2430..84a878a97a 100644 --- a/skills/faceless-explainer/scripts/audio.mjs +++ b/skills/faceless-explainer/scripts/audio.mjs @@ -105,6 +105,11 @@ function toProductLaunchMeta(neutral) { duration_s: neutral.bgm.duration_s ?? null, } : null; + // bgm_pending must survive the neutral → skill translation. A detached generate + // (Lyria/MusicGen) leaves `bgm: null, bgm_pending: true` until the track lands; dropping the + // flag makes "not ready yet" indistinguishable from "silent by design", so a later + // `fetch-sfx` snapshot turns a still-generating bed into no music at all with no signal. + const bgmPending = !!neutral.bgm_pending; const sfx = (neutral.sfx ?? []).map((s) => ({ frame: Number(s.id), file: s.file, @@ -112,7 +117,7 @@ function toProductLaunchMeta(neutral) { duration_s: s.duration_s ?? 1, volume: s.volume ?? 0.35, })); - return { bgm, voices, sfx }; + return { bgm, bgm_pending: bgmPending, voices, sfx }; } // ── generate (TTS + BGM) ──────────────────────────────────────────────────── @@ -193,12 +198,16 @@ function runFetchSfx(argv) { const manifest = parseStoryboard(readFileSync(storyboardPath, "utf8")); // Per-frame `sfx:` cues (comma-separated) → engine lines carrying only sfx. + // `filter(Boolean)` alone is not enough: a storyboard that spells "no SFX here" as + // `sfx: none` reaches the engine as a cue literally NAMED "none", which then fails to + // resolve. The absence sentinels are part of the storyboard vocabulary, so drop them. + const SFX_NONE = new Set(["none", "no", "n/a", "na", "skip", "-", "—", "–"]); const lines = []; for (const f of manifest.frames) { const names = (f.extra?.sfx ?? "") .split(",") .map((s) => s.trim()) - .filter(Boolean); + .filter((s) => s && !SFX_NONE.has(s.toLowerCase())); if (names.length && f.number != null) lines.push({ id: pad2(f.number), sfx: names }); } @@ -212,6 +221,15 @@ function runFetchSfx(argv) { const meta = toProductLaunchMeta(JSON.parse(readFileSync(neutral, "utf8"))); writeFileSync(outPath, JSON.stringify(meta, null, 2)); console.log(`✓ audio fetch-sfx: ${meta.sfx.length} SFX cue(s) → ${outPath}`); + // This pass rewrites audio_meta.json from the neutral sidecar. If a detached BGM generate is + // still running, the bed it eventually writes is NOT folded back in — the snapshot we just + // took has no music. Say so instead of leaving a silent film behind. + if (meta.bgm_pending && !meta.bgm) { + console.warn( + "⚠ audio fetch-sfx: a detached BGM generate is still pending, so this snapshot has no bed. " + + "Re-run `fetch-sfx` (or re-point audio_meta.json at the track) once it lands, before assembling.", + ); + } } // ── sync-durations (local; rewrites STORYBOARD.md) ──────────────────────────── diff --git a/skills/faceless-explainer/scripts/audio.test.mjs b/skills/faceless-explainer/scripts/audio.test.mjs index a7731f108b..cdad04f621 100644 --- a/skills/faceless-explainer/scripts/audio.test.mjs +++ b/skills/faceless-explainer/scripts/audio.test.mjs @@ -89,3 +89,88 @@ test("a storyboard music mood still retrieves BGM (marker is exact, not fuzzy)", assert.equal(request.bgm.mode, "retrieve"); assert.equal(request.bgm.query, "upbeat synthwave with heavy drums"); }); + +// ── fetch-sfx ──────────────────────────────────────────────────────────────── +// Regressions found while running the full product-launch workflow end to end +// (linear.app site showcase, 2026-07-30). + +/** Runs the fetch-sfx subcommand. `neutralOut` is what the stub engine writes to --out. */ +function runFetchSfx({ storyboard, neutralOut = { voices: [], bgm: null, sfx: [] } }) { + const dir = mkdtempSync(join(tmpdir(), "product-launch-sfx-")); + const engine = join(dir, "engine.mjs"); + writeFileSync(join(dir, "STORYBOARD.md"), storyboard); + writeFileSync( + engine, + `import { readFileSync, writeFileSync } from "node:fs"; +const argv = process.argv.slice(2); +const flag = (name) => argv[argv.indexOf(name) + 1]; +const request = JSON.parse(readFileSync(flag("--request"), "utf8")); +writeFileSync(new URL("request.json", import.meta.url), JSON.stringify(request)); +writeFileSync(flag("--out"), ${JSON.stringify(JSON.stringify(neutralOut))}); +`, + ); + const result = spawnSync( + process.execPath, + [script, "fetch-sfx", "--hyperframes", dir, "--storyboard", join(dir, "STORYBOARD.md")], + { encoding: "utf8", env: { ...process.env, HF_MEDIA_ENGINE: engine } }, + ); + return { dir, result }; +} + +const FRAME_WITH_SFX = (sfx) => + `---\nmessage: Test\n---\n\n## Frame 1 — Hook\n- duration: 3s\n- sfx: ${sfx}\n`; + +test("fetch-sfx: `sfx: none` is an absence marker, not a cue named none", () => { + const { dir, result } = runFetchSfx({ storyboard: FRAME_WITH_SFX("none") }); + + assert.equal(result.status, 0, result.stderr); + const request = JSON.parse(readFileSync(join(dir, "request.json"), "utf8")); + // Used to reach the engine as { sfx: ["none"] } — a cue that cannot resolve. + assert.deepEqual(request.lines, []); +}); + +test("fetch-sfx: the other absence spellings are markers too", () => { + for (const spelling of ["None", "n/a", "NA", "skip", "-", "—"]) { + const { dir, result } = runFetchSfx({ storyboard: FRAME_WITH_SFX(spelling) }); + assert.equal(result.status, 0, result.stderr); + const request = JSON.parse(readFileSync(join(dir, "request.json"), "utf8")); + assert.deepEqual(request.lines, [], `spelling: ${spelling}`); + } +}); + +test("fetch-sfx: a real cue still reaches the engine, and mixed lists drop only the marker", () => { + const { dir, result } = runFetchSfx({ storyboard: FRAME_WITH_SFX("whoosh, none, click") }); + + assert.equal(result.status, 0, result.stderr); + const request = JSON.parse(readFileSync(join(dir, "request.json"), "utf8")); + assert.deepEqual(request.lines, [{ id: "01", sfx: ["whoosh", "click"] }]); +}); + +test("fetch-sfx: carries bgm_pending through and warns that the snapshot has no bed", () => { + const { dir, result } = runFetchSfx({ + storyboard: FRAME_WITH_SFX("whoosh"), + // A detached Lyria/MusicGen generate that has not landed yet. + neutralOut: { voices: [], bgm: null, bgm_pending: true, sfx: [] }, + }); + + assert.equal(result.status, 0, result.stderr); + const meta = JSON.parse(readFileSync(join(dir, "audio_meta.json"), "utf8")); + // The flag used to be dropped in the neutral → PL translation, making "not ready yet" + // indistinguishable from "silent by design". + assert.equal(meta.bgm_pending, true); + assert.equal(meta.bgm, null); + assert.match(result.stderr + result.stdout, /pending/i); +}); + +test("fetch-sfx: a resolved bed reports bgm_pending false and no warning", () => { + const { dir, result } = runFetchSfx({ + storyboard: FRAME_WITH_SFX("whoosh"), + neutralOut: { voices: [], bgm: { path: "assets/bgm/track.mp3", volume: 0.12 }, sfx: [] }, + }); + + assert.equal(result.status, 0, result.stderr); + const meta = JSON.parse(readFileSync(join(dir, "audio_meta.json"), "utf8")); + assert.equal(meta.bgm_pending, false); + assert.equal(meta.bgm.path, "assets/bgm/track.mp3"); + assert.doesNotMatch(result.stderr, /pending/i); +}); diff --git a/skills/pr-to-video/scripts/audio.mjs b/skills/pr-to-video/scripts/audio.mjs index 3bae9e2430..84a878a97a 100644 --- a/skills/pr-to-video/scripts/audio.mjs +++ b/skills/pr-to-video/scripts/audio.mjs @@ -105,6 +105,11 @@ function toProductLaunchMeta(neutral) { duration_s: neutral.bgm.duration_s ?? null, } : null; + // bgm_pending must survive the neutral → skill translation. A detached generate + // (Lyria/MusicGen) leaves `bgm: null, bgm_pending: true` until the track lands; dropping the + // flag makes "not ready yet" indistinguishable from "silent by design", so a later + // `fetch-sfx` snapshot turns a still-generating bed into no music at all with no signal. + const bgmPending = !!neutral.bgm_pending; const sfx = (neutral.sfx ?? []).map((s) => ({ frame: Number(s.id), file: s.file, @@ -112,7 +117,7 @@ function toProductLaunchMeta(neutral) { duration_s: s.duration_s ?? 1, volume: s.volume ?? 0.35, })); - return { bgm, voices, sfx }; + return { bgm, bgm_pending: bgmPending, voices, sfx }; } // ── generate (TTS + BGM) ──────────────────────────────────────────────────── @@ -193,12 +198,16 @@ function runFetchSfx(argv) { const manifest = parseStoryboard(readFileSync(storyboardPath, "utf8")); // Per-frame `sfx:` cues (comma-separated) → engine lines carrying only sfx. + // `filter(Boolean)` alone is not enough: a storyboard that spells "no SFX here" as + // `sfx: none` reaches the engine as a cue literally NAMED "none", which then fails to + // resolve. The absence sentinels are part of the storyboard vocabulary, so drop them. + const SFX_NONE = new Set(["none", "no", "n/a", "na", "skip", "-", "—", "–"]); const lines = []; for (const f of manifest.frames) { const names = (f.extra?.sfx ?? "") .split(",") .map((s) => s.trim()) - .filter(Boolean); + .filter((s) => s && !SFX_NONE.has(s.toLowerCase())); if (names.length && f.number != null) lines.push({ id: pad2(f.number), sfx: names }); } @@ -212,6 +221,15 @@ function runFetchSfx(argv) { const meta = toProductLaunchMeta(JSON.parse(readFileSync(neutral, "utf8"))); writeFileSync(outPath, JSON.stringify(meta, null, 2)); console.log(`✓ audio fetch-sfx: ${meta.sfx.length} SFX cue(s) → ${outPath}`); + // This pass rewrites audio_meta.json from the neutral sidecar. If a detached BGM generate is + // still running, the bed it eventually writes is NOT folded back in — the snapshot we just + // took has no music. Say so instead of leaving a silent film behind. + if (meta.bgm_pending && !meta.bgm) { + console.warn( + "⚠ audio fetch-sfx: a detached BGM generate is still pending, so this snapshot has no bed. " + + "Re-run `fetch-sfx` (or re-point audio_meta.json at the track) once it lands, before assembling.", + ); + } } // ── sync-durations (local; rewrites STORYBOARD.md) ──────────────────────────── diff --git a/skills/product-launch-video/scripts/assemble-index.mjs b/skills/product-launch-video/scripts/assemble-index.mjs index c84d0a48ae..79bebf8480 100644 --- a/skills/product-launch-video/scripts/assemble-index.mjs +++ b/skills/product-launch-video/scripts/assemble-index.mjs @@ -476,7 +476,14 @@ let audio = { bgm: null, voices: [], sfx: [] }; if (existsSync(audioMetaPath)) { try { const parsed = JSON.parse(readFileSync(audioMetaPath, "utf8")); - audio = { bgm: parsed.bgm ?? null, voices: parsed.voices ?? [], sfx: parsed.sfx ?? [] }; + // bgm_pending rides along: without it this step cannot tell a detached generate that has + // not landed yet from a film that is silent by design, and it would build the silent one. + audio = { + bgm: parsed.bgm ?? null, + bgm_pending: !!parsed.bgm_pending, + voices: parsed.voices ?? [], + sfx: parsed.sfx ?? [], + }; } catch (e) { die(`audio_meta.json parse: ${e.message}`); } @@ -578,6 +585,14 @@ if (audio.bgm?.path) { } else { 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. + 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.", + ); } // (track 2) captions — captions.mjs writes this or legally skips; key off existence. From 6b9307786fb1f733342765065227a13bb962df95 Mon Sep 17 00:00:00 2001 From: Miao Yang Date: Thu, 30 Jul 2026 17:55:07 +0800 Subject: [PATCH 5/6] fix(capture,audio): meet the two review asks I under-delivered on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to 194fb6995. 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.** 194fb6995 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 --- .../cli/src/capture/screenshotCapture.test.ts | 39 ++++++++-- packages/cli/src/capture/screenshotCapture.ts | 19 ++--- skills-manifest.json | 4 +- .../scripts/assemble-index.mjs | 20 ++++-- .../scripts/assemble-index.test.mjs | 72 +++++++++++++++++++ 5 files changed, 132 insertions(+), 22 deletions(-) create mode 100644 skills/product-launch-video/scripts/assemble-index.test.mjs diff --git a/packages/cli/src/capture/screenshotCapture.test.ts b/packages/cli/src/capture/screenshotCapture.test.ts index d8dc14569b..f137b8657f 100644 --- a/packages/cli/src/capture/screenshotCapture.test.ts +++ b/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"); + }); +}); diff --git a/packages/cli/src/capture/screenshotCapture.ts b/packages/cli/src/capture/screenshotCapture.ts index f574f4fd65..41b09b8c52 100644 --- a/packages/cli/src/capture/screenshotCapture.ts +++ b/packages/cli/src/capture/screenshotCapture.ts @@ -60,15 +60,6 @@ export async function captureFullPagePlate( page: Page, screenshotsDir: string, ): Promise { - // 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 diff --git a/skills-manifest.json b/skills-manifest.json index 8284f6962d..390c2888ed 100644 --- a/skills-manifest.json +++ b/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", diff --git a/skills/product-launch-video/scripts/assemble-index.mjs b/skills/product-launch-video/scripts/assemble-index.mjs index 79bebf8480..b8f92f4078 100644 --- a/skills/product-launch-video/scripts/assemble-index.mjs +++ b/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", ); } diff --git a/skills/product-launch-video/scripts/assemble-index.test.mjs b/skills/product-launch-video/scripts/assemble-index.test.mjs new file mode 100644 index 0000000000..bd60c87115 --- /dev/null +++ b/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"), + '
' + + '
', + ); + 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/); +}); From 3a6c173e7d7c21e319f6c9879210ad47db1ac6cb Mon Sep 17 00:00:00 2001 From: Miao Yang Date: Thu, 30 Jul 2026 18:15:11 +0800 Subject: [PATCH 6/6] fix(audio): carry the bgm_pending gate into the sibling assemblers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The remaining blocker, and one this PR created: the previous commit made all three copies of the audio adapter *emit* bgm_pending, but only product-launch-video's assembler *reads* it. So faceless-explainer and pr-to-video would do exactly what this PR set out to stop — parse an audio_meta.json that says the bed is still generating and assemble the silent film without a word. Producer fixed in three places, consumer in one, is worse than neither: before this PR there was no flag to drop. Both siblings now get the same three changes product-launch-video got — the flag carried through the audio object, `die` on `bgm_pending && !bgm`, and `--allow-pending-bgm` as the deliberate escape — plus the same three tests: refusal writes no index.html, the escape assembles and says so, and a film that is silent *by design* still assembles untouched. That last one is the one worth having; it proves the flag restored a distinction rather than just adding a gate. Applied as three separate patches rather than a file copy: these assemblers have diverged (pr-to-video validates a bare `