Skip to content

Commit 7085f60

Browse files
fix(capture,audio): three defects found running product-launch-video end to end
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 `<svg>`'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 `<img src="logo-abc.svg">` 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
1 parent 5244dde commit 7085f60

5 files changed

Lines changed: 174 additions & 6 deletions

File tree

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

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { afterEach, describe, expect, it, vi } from "vitest";
2-
import { isPrivateUrl, safeFetch } from "./assetDownloader.js";
2+
import { isPrivateUrl, safeFetch, toStandaloneSvg } from "./assetDownloader.js";
33

44
describe("isPrivateUrl — SSRF denylist (security: F-003)", () => {
55
it("blocks loopback, private, and metadata IPv4", () => {
@@ -91,3 +91,39 @@ describe("safeFetch — re-validates the denylist on every redirect hop (securit
9191
expect(fetchMock).not.toHaveBeenCalled();
9292
});
9393
});
94+
95+
describe("toStandaloneSvg — scraped inline SVGs must survive as .svg files", () => {
96+
it("adds the SVG namespace that outerHTML omits for inline SVG", () => {
97+
const inline = '<svg viewBox="0 0 24 24"><path d="M0 0h24v24H0z"/></svg>';
98+
const out = toStandaloneSvg(inline);
99+
expect(out).toContain('xmlns="http://www.w3.org/2000/svg"');
100+
// Nothing else may change — the path geometry is the brand mark.
101+
expect(out).toContain('<path d="M0 0h24v24H0z"/>');
102+
expect(out.endsWith("</svg>")).toBe(true);
103+
});
104+
105+
it("leaves an SVG that already declares xmlns untouched", () => {
106+
const already = '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 8 8"><rect/></svg>';
107+
expect(toStandaloneSvg(already)).toBe(already);
108+
});
109+
110+
it("declares xmlns:xlink only when an xlink: attribute is actually used", () => {
111+
const withXlink = '<svg viewBox="0 0 8 8"><use xlink:href="#a"/></svg>';
112+
expect(toStandaloneSvg(withXlink)).toContain('xmlns:xlink="http://www.w3.org/1999/xlink"');
113+
const without = '<svg viewBox="0 0 8 8"><use href="#a"/></svg>';
114+
expect(toStandaloneSvg(without)).not.toContain("xmlns:xlink");
115+
});
116+
117+
it("is idempotent and preserves attributes on the root", () => {
118+
const inline = '<svg class="logo" width="120" height="24" fill="currentColor"><g/></svg>';
119+
const once = toStandaloneSvg(inline);
120+
expect(toStandaloneSvg(once)).toBe(once);
121+
for (const attr of ['class="logo"', 'width="120"', 'height="24"', 'fill="currentColor"']) {
122+
expect(once).toContain(attr);
123+
}
124+
});
125+
126+
it("returns non-SVG input unchanged rather than corrupting it", () => {
127+
expect(toStandaloneSvg("<div>not an svg</div>")).toBe("<div>not an svg</div>");
128+
});
129+
});

packages/cli/src/capture/assetDownloader.ts

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,32 @@ function svgContentHashSlug(svgSource: string | Buffer, isLogo: boolean): string
1717
return isLogo ? `logo-${hash}` : `svg-${hash}`;
1818
}
1919

20+
/**
21+
* Make a scraped inline `<svg>` usable as a standalone `.svg` file.
22+
*
23+
* An inline SVG in an HTML document inherits the SVG namespace from the parser, so the DOM's
24+
* `outerHTML` does not serialize `xmlns`. That string is fine pasted back into HTML but is NOT
25+
* a valid standalone document: `<img src="logo-abc123.svg">` renders a broken-image icon, which
26+
* is how these assets are actually consumed downstream. Declare the namespaces on the way to disk.
27+
*
28+
* `xlink:href` is deprecated but still emitted by plenty of sites; an undeclared `xlink:` prefix
29+
* is a parse error in a standalone document, so declare that too — but only when it is used.
30+
*/
31+
export function toStandaloneSvg(outerHTML: string): string {
32+
const open = outerHTML.match(/<svg\b[^>]*>/i);
33+
if (!open) return outerHTML;
34+
const original = open[0];
35+
let tag = original;
36+
const add: string[] = [];
37+
if (!/\sxmlns\s*=/i.test(tag)) add.push('xmlns="http://www.w3.org/2000/svg"');
38+
if (/\sxlink:[a-z-]+\s*=/i.test(outerHTML) && !/\sxmlns:xlink\s*=/i.test(tag)) {
39+
add.push('xmlns:xlink="http://www.w3.org/1999/xlink"');
40+
}
41+
if (!add.length) return outerHTML;
42+
tag = tag.replace(/^<svg\b/i, `<svg ${add.join(" ")}`);
43+
return outerHTML.replace(original, tag);
44+
}
45+
2046
export async function downloadAssets(
2147
tokens: DesignTokens,
2248
outputDir: string,
@@ -34,7 +60,9 @@ export async function downloadAssets(
3460
for (let i = 0; i < tokens.svgs.length && i < 30; i++) {
3561
const svg = tokens.svgs[i]!;
3662
if (!svg.outerHTML || svg.outerHTML.length < 50) continue;
37-
const slug = svgContentHashSlug(svg.outerHTML, !!svg.isLogo);
63+
// Hash the bytes that actually land on disk, so the filename still can't drift from content.
64+
const svgFile = toStandaloneSvg(svg.outerHTML);
65+
const slug = svgContentHashSlug(svgFile, !!svg.isLogo);
3866
let finalSlug = slug;
3967
let suffix = 2;
4068
while (usedSvgNames.has(finalSlug)) {
@@ -45,7 +73,7 @@ export async function downloadAssets(
4573
const name = `${finalSlug}.svg`;
4674
const localPath = `assets/svgs/${name}`;
4775
try {
48-
writeFileSync(join(outputDir, localPath), svg.outerHTML, "utf-8");
76+
writeFileSync(join(outputDir, localPath), svgFile, "utf-8");
4977
assets.push({ url: "", localPath, type: "svg" });
5078
} catch {
5179
/* skip */

skills-manifest.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@
6262
"files": 29
6363
},
6464
"product-launch-video": {
65-
"hash": "1a14737e16f6a154",
65+
"hash": "6f2b690d22392425",
6666
"files": 26
6767
},
6868
"remotion-to-hyperframes": {

skills/product-launch-video/scripts/audio.mjs

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,14 +103,19 @@ function toProductLaunchMeta(neutral) {
103103
duration_s: neutral.bgm.duration_s ?? null,
104104
}
105105
: null;
106+
// bgm_pending must survive the neutral → PL translation. A detached generate (Lyria/MusicGen)
107+
// leaves `bgm: null, bgm_pending: true` until the track lands; dropping the flag made
108+
// "not ready yet" indistinguishable from "silent by design", so a later `fetch-sfx` snapshot
109+
// turned a still-generating bed into no music at all with nothing to signal it.
110+
const bgmPending = !!neutral.bgm_pending;
106111
const sfx = (neutral.sfx ?? []).map((s) => ({
107112
frame: Number(s.id),
108113
file: s.file,
109114
offset_s: s.offset_s ?? 0,
110115
duration_s: s.duration_s ?? 1,
111116
volume: s.volume ?? 0.35,
112117
}));
113-
return { bgm, voices, sfx };
118+
return { bgm, bgm_pending: bgmPending, voices, sfx };
114119
}
115120

116121
// ── generate (TTS + BGM) ────────────────────────────────────────────────────
@@ -192,12 +197,16 @@ function runFetchSfx(argv) {
192197
const manifest = parseStoryboard(readFileSync(storyboardPath, "utf8"));
193198

194199
// Per-frame `sfx:` cues (comma-separated) → engine lines carrying only sfx.
200+
// `filter(Boolean)` alone is not enough: a storyboard that spells "no SFX here" as
201+
// `sfx: none` used to reach the engine as a cue literally NAMED "none", which then failed
202+
// to resolve. The absence sentinels are part of the storyboard vocabulary, so drop them.
203+
const SFX_NONE = new Set(["none", "no", "n/a", "na", "skip", "-", "—", "–"]);
195204
const lines = [];
196205
for (const f of manifest.frames) {
197206
const names = (f.extra?.sfx ?? "")
198207
.split(",")
199208
.map((s) => s.trim())
200-
.filter(Boolean);
209+
.filter((s) => s && !SFX_NONE.has(s.toLowerCase()));
201210
if (names.length && f.number != null) lines.push({ id: pad2(f.number), sfx: names });
202211
}
203212

@@ -211,6 +220,16 @@ function runFetchSfx(argv) {
211220
const meta = toProductLaunchMeta(JSON.parse(readFileSync(neutral, "utf8")));
212221
writeFileSync(outPath, JSON.stringify(meta, null, 2));
213222
console.log(`✓ audio fetch-sfx: ${meta.sfx.length} SFX cue(s) → ${outPath}`);
223+
// This pass rewrites audio_meta.json from the neutral sidecar. If a detached BGM generate is
224+
// still running, the bed it eventually writes is NOT folded back in — the snapshot we just
225+
// took has no music. Say so instead of leaving a silent film that the storyboard claims has a
226+
// bed (observed live: the caller had to notice on its own and rebuild the entry).
227+
if (meta.bgm_pending && !meta.bgm) {
228+
console.warn(
229+
"⚠ audio fetch-sfx: a detached BGM generate is still pending, so this snapshot has no bed. " +
230+
"Re-run `fetch-sfx` (or re-point audio_meta.json at the track) once it lands, before assembling.",
231+
);
232+
}
214233
}
215234

216235
// ── sync-durations (local; rewrites STORYBOARD.md) ────────────────────────────

skills/product-launch-video/scripts/audio.test.mjs

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,3 +131,88 @@ test("a storyboard music mood still retrieves BGM (marker is exact, not fuzzy)",
131131
assert.equal(request.bgm.mode, "retrieve");
132132
assert.equal(request.bgm.query, "upbeat synthwave with heavy drums");
133133
});
134+
135+
// ── fetch-sfx ────────────────────────────────────────────────────────────────
136+
// Regressions found while running the full product-launch workflow end to end
137+
// (linear.app site showcase, 2026-07-30).
138+
139+
/** Runs the fetch-sfx subcommand. `neutralOut` is what the stub engine writes to --out. */
140+
function runFetchSfx({ storyboard, neutralOut = { voices: [], bgm: null, sfx: [] } }) {
141+
const dir = mkdtempSync(join(tmpdir(), "product-launch-sfx-"));
142+
const engine = join(dir, "engine.mjs");
143+
writeFileSync(join(dir, "STORYBOARD.md"), storyboard);
144+
writeFileSync(
145+
engine,
146+
`import { readFileSync, writeFileSync } from "node:fs";
147+
const argv = process.argv.slice(2);
148+
const flag = (name) => argv[argv.indexOf(name) + 1];
149+
const request = JSON.parse(readFileSync(flag("--request"), "utf8"));
150+
writeFileSync(new URL("request.json", import.meta.url), JSON.stringify(request));
151+
writeFileSync(flag("--out"), ${JSON.stringify(JSON.stringify(neutralOut))});
152+
`,
153+
);
154+
const result = spawnSync(
155+
process.execPath,
156+
[script, "fetch-sfx", "--hyperframes", dir, "--storyboard", join(dir, "STORYBOARD.md")],
157+
{ encoding: "utf8", env: { ...process.env, HF_MEDIA_ENGINE: engine } },
158+
);
159+
return { dir, result };
160+
}
161+
162+
const FRAME_WITH_SFX = (sfx) =>
163+
`---\nmessage: Test\n---\n\n## Frame 1 — Hook\n- duration: 3s\n- sfx: ${sfx}\n`;
164+
165+
test("fetch-sfx: `sfx: none` is an absence marker, not a cue named none", () => {
166+
const { dir, result } = runFetchSfx({ storyboard: FRAME_WITH_SFX("none") });
167+
168+
assert.equal(result.status, 0, result.stderr);
169+
const request = JSON.parse(readFileSync(join(dir, "request.json"), "utf8"));
170+
// Used to reach the engine as { sfx: ["none"] } — a cue that cannot resolve.
171+
assert.deepEqual(request.lines, []);
172+
});
173+
174+
test("fetch-sfx: the other absence spellings are markers too", () => {
175+
for (const spelling of ["None", "n/a", "NA", "skip", "-", "—"]) {
176+
const { dir, result } = runFetchSfx({ storyboard: FRAME_WITH_SFX(spelling) });
177+
assert.equal(result.status, 0, result.stderr);
178+
const request = JSON.parse(readFileSync(join(dir, "request.json"), "utf8"));
179+
assert.deepEqual(request.lines, [], `spelling: ${spelling}`);
180+
}
181+
});
182+
183+
test("fetch-sfx: a real cue still reaches the engine, and mixed lists drop only the marker", () => {
184+
const { dir, result } = runFetchSfx({ storyboard: FRAME_WITH_SFX("whoosh, none, click") });
185+
186+
assert.equal(result.status, 0, result.stderr);
187+
const request = JSON.parse(readFileSync(join(dir, "request.json"), "utf8"));
188+
assert.deepEqual(request.lines, [{ id: "01", sfx: ["whoosh", "click"] }]);
189+
});
190+
191+
test("fetch-sfx: carries bgm_pending through and warns that the snapshot has no bed", () => {
192+
const { dir, result } = runFetchSfx({
193+
storyboard: FRAME_WITH_SFX("whoosh"),
194+
// A detached Lyria/MusicGen generate that has not landed yet.
195+
neutralOut: { voices: [], bgm: null, bgm_pending: true, sfx: [] },
196+
});
197+
198+
assert.equal(result.status, 0, result.stderr);
199+
const meta = JSON.parse(readFileSync(join(dir, "audio_meta.json"), "utf8"));
200+
// The flag used to be dropped in the neutral → PL translation, making "not ready yet"
201+
// indistinguishable from "silent by design".
202+
assert.equal(meta.bgm_pending, true);
203+
assert.equal(meta.bgm, null);
204+
assert.match(result.stderr + result.stdout, /pending/i);
205+
});
206+
207+
test("fetch-sfx: a resolved bed reports bgm_pending false and no warning", () => {
208+
const { dir, result } = runFetchSfx({
209+
storyboard: FRAME_WITH_SFX("whoosh"),
210+
neutralOut: { voices: [], bgm: { path: "assets/bgm/track.mp3", volume: 0.12 }, sfx: [] },
211+
});
212+
213+
assert.equal(result.status, 0, result.stderr);
214+
const meta = JSON.parse(readFileSync(join(dir, "audio_meta.json"), "utf8"));
215+
assert.equal(meta.bgm_pending, false);
216+
assert.equal(meta.bgm.path, "assets/bgm/track.mp3");
217+
assert.doesNotMatch(result.stderr, /pending/i);
218+
});

0 commit comments

Comments
 (0)