mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-01 19:42:03 +00:00
fix(skills,producer): terminate ffprobe options in shipped skill scripts
The contract test only walked packages/*/src and only .ts, so it could not see
the shipped agent tools under skills/**, which are .mjs/.cjs. 19 call sites
there and in package tests were still missing `--` immediately before the
input while the suite reported the bug class closed — a dash-prefixed filename
is parsed as an option and fails the same way.
Sweeps packages/, skills/ and scripts/ now, including .mjs/.cjs and test
files (dither.test.mjs was one of the broken sites). Excludes only the
contract test itself, which documents the contract with example argvs
including a deliberately misordered one.
Two guards were fixed while widening: the terminator must never be inserted
after `-i`, which consumes the next token (a blind pass hit an ffmpeg input
and a base64 -i), and comment prose describing a spawn is not a spawn.
Also routes every audioPadTrim probe failure through one sanitizer at the
boundary. runFfprobeJson scrubbed its own stderr, but
defaultProbeVideoFrameInfo threw `no video stream in ${videoPath}` raw into
the public PadTrimAudioResult.error, and an injected probe can throw anything.
The redaction unit tests all passed with the caller wiring deleted; the new
public-path regressions fail without it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
e79ab3ab31
commit
255cf92915
@@ -150,6 +150,7 @@ function probeStream(
|
||||
"-count_packets",
|
||||
"-of",
|
||||
"json",
|
||||
"--",
|
||||
outputPath,
|
||||
],
|
||||
{ stdio: "pipe" },
|
||||
@@ -418,6 +419,7 @@ describe("assemble()", () => {
|
||||
"stream=r_frame_rate,avg_frame_rate,duration",
|
||||
"-of",
|
||||
"json",
|
||||
"--",
|
||||
outputPath,
|
||||
],
|
||||
{ stdio: "pipe" },
|
||||
|
||||
@@ -67,6 +67,7 @@ describe.skipIf(!hasFfmpeg)("audio pad real-media packet contract", () => {
|
||||
"packet=duration_time",
|
||||
"-of",
|
||||
"json",
|
||||
"--",
|
||||
output,
|
||||
],
|
||||
{ encoding: "utf8" },
|
||||
|
||||
@@ -270,3 +270,71 @@ describe("padOrTrimAudioToVideoFrameCount", () => {
|
||||
expect(result.targetDurationSeconds).toBe(6);
|
||||
});
|
||||
});
|
||||
|
||||
// ── Public-path path redaction ────────────────────────────────────────────
|
||||
//
|
||||
// The redaction helpers have their own unit tests, but those pass whether or
|
||||
// not this module actually CALLS them: deleting the wiring in
|
||||
// padOrTrimAudioToVideoFrameCount left every one of them green. These drive
|
||||
// the public entry point and assert on the public `PadTrimAudioResult.error`,
|
||||
// which is what reaches logs, telemetry, and the caller.
|
||||
describe("PadTrimAudioResult.error never carries the input path", () => {
|
||||
const cases: Array<{ name: string; videoPath: string; secret: string }> = [
|
||||
{
|
||||
name: "a dash-prefixed relative path",
|
||||
videoPath: "./assets/-customer-secret-intro.mp4",
|
||||
secret: "customer-secret-intro",
|
||||
},
|
||||
{
|
||||
name: "a non-allowlisted absolute root",
|
||||
videoPath: "/data/acme-secret/video.mp4",
|
||||
secret: "acme-secret",
|
||||
},
|
||||
{
|
||||
name: "a bare relative path",
|
||||
videoPath: "customer/acme-secret/video.mp4",
|
||||
secret: "acme-secret",
|
||||
},
|
||||
];
|
||||
|
||||
for (const { name, videoPath, secret } of cases) {
|
||||
it(`redacts ${name} raised by the video probe`, async () => {
|
||||
const result = await padOrTrimAudioToVideoFrameCount({
|
||||
videoPath,
|
||||
audioPath: "/tmp/audio.aac",
|
||||
outputPath: "/tmp/out.aac",
|
||||
// Reproduces the real thrower: defaultProbeVideoFrameInfo raises
|
||||
// `ffprobe found no video stream in ${videoPath}` with the raw path.
|
||||
probeVideoFrameInfo: () =>
|
||||
Promise.reject(new Error(`ffprobe found no video stream in ${videoPath}`)),
|
||||
probeAudioInfo: () => Promise.resolve({ durationSeconds: 1 }),
|
||||
runFfmpeg: () => Promise.resolve({ success: true }),
|
||||
});
|
||||
|
||||
expect(result.success).toBe(false);
|
||||
expect(result.error).toBeDefined();
|
||||
expect(result.error ?? "").not.toContain(secret);
|
||||
expect(result.error ?? "").not.toContain(videoPath);
|
||||
// Still diagnosable — the failure mode survives redaction.
|
||||
expect(result.error ?? "").toContain("failed to probe video");
|
||||
});
|
||||
}
|
||||
|
||||
it("redacts raw ffprobe stderr surfaced through the audio probe", async () => {
|
||||
const result = await padOrTrimAudioToVideoFrameCount({
|
||||
videoPath: "/tmp/v.mp4",
|
||||
audioPath: "/data/acme-secret/audio.aac",
|
||||
outputPath: "/tmp/out.aac",
|
||||
probeVideoFrameInfo: () => Promise.resolve({ frameCount: 30, fpsNum: 30, fpsDen: 1 }),
|
||||
probeAudioInfo: () =>
|
||||
Promise.reject(
|
||||
new Error("/data/acme-secret/audio.aac: Invalid data found when processing input"),
|
||||
),
|
||||
runFfmpeg: () => Promise.resolve({ success: true }),
|
||||
});
|
||||
|
||||
expect(result.success).toBe(false);
|
||||
expect(result.error ?? "").not.toContain("acme-secret");
|
||||
expect(result.error ?? "").toContain("failed to probe audio");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -215,6 +215,25 @@ function formatSeconds(sec: number): string {
|
||||
return sec.toFixed(6);
|
||||
}
|
||||
|
||||
/**
|
||||
* Every probe failure message, sanitized once, at the one place they all pass
|
||||
* through on their way into the public `PadTrimAudioResult.error`.
|
||||
*
|
||||
* `runFfprobeJson` already scrubs the stderr it raises, but it is not the only
|
||||
* thrower: `defaultProbeVideoFrameInfo` raises
|
||||
* `ffprobe found no video stream in ${videoPath}` with the raw path, and a
|
||||
* caller-supplied `probeVideoFrameInfo` / `probeAudioInfo` can raise anything
|
||||
* at all. Sanitizing per-thrower is a list that will drift; sanitizing at the
|
||||
* boundary cannot be bypassed by adding a new throw upstream.
|
||||
*
|
||||
* Known paths first (this function has them in hand, so no pattern has to
|
||||
* recognise them), then the generic shape-based scrub for anything the message
|
||||
* picked up elsewhere.
|
||||
*/
|
||||
function sanitizeProbeFailure(message: string, paths: readonly string[]): string {
|
||||
return redactTelemetryString(redactKnownPaths(message, paths));
|
||||
}
|
||||
|
||||
/**
|
||||
* Pad or trim `audio.aac` so its exact duration matches `frameCount / fps`
|
||||
* for the assembled video.
|
||||
@@ -235,12 +254,16 @@ export async function padOrTrimAudioToVideoFrameCount(
|
||||
probeAudio(input.audioPath, input.signal),
|
||||
]);
|
||||
|
||||
const probePaths = [input.videoPath, input.audioPath, input.outputPath];
|
||||
if (videoResult.status === "rejected") {
|
||||
return failResult(
|
||||
input.outputPath,
|
||||
0,
|
||||
audioResult.status === "fulfilled" ? audioResult.value.durationSeconds : 0,
|
||||
`audioPadTrim: failed to probe video: ${(videoResult.reason as Error).message}`,
|
||||
`audioPadTrim: failed to probe video: ${sanitizeProbeFailure(
|
||||
(videoResult.reason as Error).message,
|
||||
probePaths,
|
||||
)}`,
|
||||
);
|
||||
}
|
||||
if (audioResult.status === "rejected") {
|
||||
@@ -248,7 +271,10 @@ export async function padOrTrimAudioToVideoFrameCount(
|
||||
input.outputPath,
|
||||
0,
|
||||
0,
|
||||
`audioPadTrim: failed to probe audio: ${(audioResult.reason as Error).message}`,
|
||||
`audioPadTrim: failed to probe audio: ${sanitizeProbeFailure(
|
||||
(audioResult.reason as Error).message,
|
||||
probePaths,
|
||||
)}`,
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -20,7 +20,19 @@ import { join, relative } from "node:path";
|
||||
* than needing to be remembered.
|
||||
*/
|
||||
const REPO_ROOT = join(import.meta.dirname, "..", "..", "..", "..");
|
||||
const PACKAGES = join(REPO_ROOT, "packages");
|
||||
|
||||
/**
|
||||
* Roots to sweep.
|
||||
*
|
||||
* `skills/` is here because leaving it out was not a scoping choice, it was a
|
||||
* hole: the shipped agent tools under `skills/**` spawn ffprobe directly, and
|
||||
* 17 of those call sites were missing the terminator while this suite reported
|
||||
* the bug class closed. They are distributed to users, not fixtures.
|
||||
*/
|
||||
const SWEEP_ROOTS = ["packages", "skills", "scripts"];
|
||||
|
||||
/** `.mjs`/`.cjs` are first-class here — the skill scripts are not TypeScript. */
|
||||
const SOURCE_EXT = /\.(?:ts|mjs|cjs|js)$/;
|
||||
|
||||
/**
|
||||
* The caller set as of the sweep that introduced this contract.
|
||||
@@ -62,15 +74,25 @@ function mentionsProbe(src: string): boolean {
|
||||
// an opaquely-named variable (`spawn(command, argv)`) is invisible here.
|
||||
// Those still get caught by argv matching whenever their flags are literal —
|
||||
// widen this if one ever slips through both.
|
||||
// Comments and doc prose describing a spawn are not a spawn:
|
||||
// `tts.test.mjs` explains `ffprobeDuration's spawnSync("ffprobe", ...) call`
|
||||
// in a comment and was reported as an unclassified caller.
|
||||
const code = src.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/[^\n]*/g, "");
|
||||
return /(?:spawn|spawnSync|execFile\w*|exec)\s*\(\s*[^,)]*(?:ffprobe|ffProbe|probeBin|probePath)/i.test(
|
||||
src,
|
||||
code,
|
||||
);
|
||||
}
|
||||
|
||||
const SKIP_DIRS = new Set(["node_modules", "dist"]);
|
||||
|
||||
function isSourceFile(entry: string): boolean {
|
||||
return entry.endsWith(".ts") && !entry.includes(".test.");
|
||||
if (!SOURCE_EXT.test(entry) || entry.endsWith(".d.ts")) return false;
|
||||
// This file documents the contract with example argvs, including a
|
||||
// deliberately misordered one. Scanning itself reports its own prose.
|
||||
if (entry === "ffprobeArgvContract.test.ts") return false;
|
||||
// Test files are swept too. A test that probes a rendered output is itself a
|
||||
// caller, and `dither.test.mjs` was one of the 17 broken sites.
|
||||
return true;
|
||||
}
|
||||
|
||||
function discoverCallers(): { found: string[]; unclassified: string[] } {
|
||||
@@ -94,12 +116,12 @@ function discoverCallers(): { found: string[]; unclassified: string[] } {
|
||||
else if (isSourceFile(entry)) classify(abs);
|
||||
}
|
||||
};
|
||||
for (const pkg of readdirSync(PACKAGES)) {
|
||||
const src = join(PACKAGES, pkg, "src");
|
||||
for (const root of SWEEP_ROOTS) {
|
||||
const abs = join(REPO_ROOT, root);
|
||||
try {
|
||||
if (statSync(src).isDirectory()) walk(src);
|
||||
if (statSync(abs).isDirectory()) walk(abs);
|
||||
} catch {
|
||||
/* package without src */
|
||||
/* root absent in a partial checkout */
|
||||
}
|
||||
}
|
||||
return { found: found.sort(), unclassified: unclassified.sort() };
|
||||
|
||||
@@ -213,6 +213,7 @@ describe("webm VP9 concat-copy smoke", () => {
|
||||
"stream=codec_name,width,height,pix_fmt,r_frame_rate",
|
||||
"-of",
|
||||
"default=noprint_wrappers=1",
|
||||
"--",
|
||||
outputPath,
|
||||
]);
|
||||
if (result.exitCode !== 0) {
|
||||
@@ -268,6 +269,7 @@ describe("webm VP9 concat-copy smoke", () => {
|
||||
"stream=nb_read_frames",
|
||||
"-of",
|
||||
"default=noprint_wrappers=1:nokey=1",
|
||||
"--",
|
||||
outputPath,
|
||||
]);
|
||||
if (result.exitCode !== 0) {
|
||||
@@ -423,6 +425,7 @@ describe("webm VP9 concat-copy smoke (yuva420p alpha)", () => {
|
||||
"-select_streams",
|
||||
"v:0",
|
||||
"-show_streams",
|
||||
"--",
|
||||
alphaOutputPath,
|
||||
]);
|
||||
expect(probeResult.exitCode).toBe(0);
|
||||
|
||||
@@ -2,15 +2,15 @@
|
||||
"source": "heygen-com/hyperframes",
|
||||
"skills": {
|
||||
"embedded-captions": {
|
||||
"hash": "ed4dc7b850b92ff5",
|
||||
"hash": "8e8bd824567c3e17",
|
||||
"files": 140
|
||||
},
|
||||
"faceless-explainer": {
|
||||
"hash": "261a9740ec1378b0",
|
||||
"hash": "c70b904aa68cf7e5",
|
||||
"files": 24
|
||||
},
|
||||
"figma": {
|
||||
"hash": "517e4dc53c13ea05",
|
||||
"hash": "4f524b4962bd8d7c",
|
||||
"files": 2
|
||||
},
|
||||
"general-video": {
|
||||
@@ -46,11 +46,11 @@
|
||||
"files": 10
|
||||
},
|
||||
"media-use": {
|
||||
"hash": "6c40be3e8bd6eacc",
|
||||
"hash": "6fedfe5fe57a9885",
|
||||
"files": 152
|
||||
},
|
||||
"motion-graphics": {
|
||||
"hash": "50db172cad89b1c7",
|
||||
"hash": "1434e22bb0259bbb",
|
||||
"files": 23
|
||||
},
|
||||
"music-to-video": {
|
||||
@@ -58,11 +58,11 @@
|
||||
"files": 132
|
||||
},
|
||||
"pr-to-video": {
|
||||
"hash": "41171bbed1c5d8f4",
|
||||
"hash": "7769801640dca521",
|
||||
"files": 30
|
||||
},
|
||||
"product-launch-video": {
|
||||
"hash": "01fc75da8492f749",
|
||||
"hash": "81953f054fcb9d91",
|
||||
"files": 28
|
||||
},
|
||||
"remotion-to-hyperframes": {
|
||||
|
||||
@@ -185,6 +185,7 @@ function main() {
|
||||
"format=duration",
|
||||
"-of",
|
||||
"default=nokey=1:noprint_wrappers=1",
|
||||
"--",
|
||||
fp,
|
||||
],
|
||||
{ encoding: "utf8" },
|
||||
|
||||
@@ -50,6 +50,7 @@ function sourceDurationSec(project) {
|
||||
"format=duration",
|
||||
"-of",
|
||||
"default=nokey=1:noprint_wrappers=1",
|
||||
"--",
|
||||
p,
|
||||
],
|
||||
{ encoding: "utf8" },
|
||||
|
||||
@@ -83,6 +83,7 @@ function probeRates(src) {
|
||||
"stream=r_frame_rate,avg_frame_rate",
|
||||
"-of",
|
||||
"default=nk=1:nw=1",
|
||||
"--",
|
||||
src,
|
||||
])
|
||||
.toString()
|
||||
|
||||
@@ -81,7 +81,7 @@ function ensureBgmCovers(relPath, hyperframesDir, total) {
|
||||
const abs = join(hyperframesDir, relPath);
|
||||
const probe = spawnSync(
|
||||
"ffprobe",
|
||||
["-v", "error", "-show_entries", "format=duration", "-of", "csv=p=0", abs],
|
||||
["-v", "error", "-show_entries", "format=duration", "-of", "csv=p=0", "--", abs],
|
||||
{ encoding: "utf8" },
|
||||
);
|
||||
if (probe.status !== 0) return { looped: false, short: false, reason: "ffprobe unavailable" };
|
||||
|
||||
@@ -51,6 +51,7 @@ const ffprobe = (file) =>
|
||||
"format=duration",
|
||||
"-of",
|
||||
"csv=p=0",
|
||||
"--",
|
||||
file,
|
||||
])
|
||||
.toString()
|
||||
@@ -69,6 +70,7 @@ const dims = execFileSync("ffprobe", [
|
||||
"stream=width,height",
|
||||
"-of",
|
||||
"csv=p=0",
|
||||
"--",
|
||||
reference,
|
||||
])
|
||||
.toString()
|
||||
|
||||
@@ -109,7 +109,7 @@ function ffmpegDurationFallback(absPath) {
|
||||
export function ffprobeDuration(absPath) {
|
||||
const r = spawnSync(
|
||||
"ffprobe",
|
||||
["-v", "error", "-show_entries", "format=duration", "-of", "default=nw=1:nk=1", absPath],
|
||||
["-v", "error", "-show_entries", "format=duration", "-of", "default=nw=1:nk=1", "--", absPath],
|
||||
{ encoding: "utf8" },
|
||||
);
|
||||
if (r.error?.code === "ENOENT") return ffmpegDurationFallback(absPath);
|
||||
|
||||
@@ -120,7 +120,7 @@ async function run() {
|
||||
function probe(filePath) {
|
||||
const raw = execFileSync(
|
||||
"ffprobe",
|
||||
["-v", "error", "-print_format", "json", "-show_streams", "-show_format", filePath],
|
||||
["-v", "error", "-print_format", "json", "-show_streams", "-show_format", "--", filePath],
|
||||
{ encoding: "utf8", timeout: 10_000 },
|
||||
);
|
||||
const parsed = JSON.parse(raw);
|
||||
|
||||
@@ -105,7 +105,7 @@ test("processes moving MP4 frames, audio, and BT.709 metadata", { skip: !HAS_FFM
|
||||
const probe = JSON.parse(
|
||||
execFileSync(
|
||||
"ffprobe",
|
||||
["-v", "error", "-print_format", "json", "-show_streams", "-show_format", output],
|
||||
["-v", "error", "-print_format", "json", "-show_streams", "-show_format", "--", output],
|
||||
{
|
||||
encoding: "utf8",
|
||||
},
|
||||
@@ -135,6 +135,7 @@ test("processes moving MP4 frames, audio, and BT.709 metadata", { skip: !HAS_FFM
|
||||
"frame=best_effort_timestamp_time",
|
||||
"-of",
|
||||
"csv=p=0",
|
||||
"--",
|
||||
output,
|
||||
],
|
||||
{ encoding: "utf8" },
|
||||
|
||||
@@ -44,6 +44,7 @@ function probeMedia(mediaPath, ffprobePath) {
|
||||
"stream=color_space,color_transfer,color_primaries,pix_fmt,duration:format=duration",
|
||||
"-of",
|
||||
"json",
|
||||
"--",
|
||||
mediaPath,
|
||||
],
|
||||
{ encoding: "utf8", timeout: 5_000, stdio: ["ignore", "pipe", "pipe"] },
|
||||
|
||||
@@ -12,7 +12,7 @@ export function probe(filePath) {
|
||||
// can't break out of the quoting — filePath is passed as a literal argv entry.
|
||||
const raw = execFileSync(
|
||||
"ffprobe",
|
||||
["-v", "quiet", "-print_format", "json", "-show_format", "-show_streams", filePath],
|
||||
["-v", "quiet", "-print_format", "json", "-show_format", "-show_streams", "--", filePath],
|
||||
{ encoding: "utf8", timeout: 5000 },
|
||||
);
|
||||
const info = JSON.parse(raw);
|
||||
|
||||
@@ -17,7 +17,7 @@ function probeDurationSeconds(file) {
|
||||
try {
|
||||
const out = execFileSync(
|
||||
"ffprobe",
|
||||
["-v", "error", "-show_entries", "format=duration", "-of", "csv=p=0", file],
|
||||
["-v", "error", "-show_entries", "format=duration", "-of", "csv=p=0", "--", file],
|
||||
{ encoding: "utf8", timeout: 15000 },
|
||||
);
|
||||
const d = parseFloat(String(out).trim());
|
||||
|
||||
@@ -209,6 +209,7 @@ function probeDuration(filePath) {
|
||||
"format=duration",
|
||||
"-of",
|
||||
"default=noprint_wrappers=1:nokey=1",
|
||||
"--",
|
||||
filePath,
|
||||
],
|
||||
{ encoding: "utf8" },
|
||||
|
||||
@@ -45,6 +45,7 @@ function probe(img) {
|
||||
"stream=width,height",
|
||||
"-of",
|
||||
"csv=p=0",
|
||||
"--",
|
||||
img,
|
||||
])
|
||||
.toString()
|
||||
|
||||
@@ -82,7 +82,7 @@ function ensureBgmCovers(relPath, hyperframesDir, total) {
|
||||
const abs = join(hyperframesDir, relPath);
|
||||
const probe = spawnSync(
|
||||
"ffprobe",
|
||||
["-v", "error", "-show_entries", "format=duration", "-of", "csv=p=0", abs],
|
||||
["-v", "error", "-show_entries", "format=duration", "-of", "csv=p=0", "--", abs],
|
||||
{ encoding: "utf8" },
|
||||
);
|
||||
if (probe.status !== 0) return { looped: false, short: false, reason: "ffprobe unavailable" };
|
||||
|
||||
@@ -81,7 +81,7 @@ function ensureBgmCovers(relPath, hyperframesDir, total) {
|
||||
const abs = join(hyperframesDir, relPath);
|
||||
const probe = spawnSync(
|
||||
"ffprobe",
|
||||
["-v", "error", "-show_entries", "format=duration", "-of", "csv=p=0", abs],
|
||||
["-v", "error", "-show_entries", "format=duration", "-of", "csv=p=0", "--", abs],
|
||||
{ encoding: "utf8" },
|
||||
);
|
||||
if (probe.status !== 0) return { looped: false, short: false, reason: "ffprobe unavailable" };
|
||||
|
||||
Reference in New Issue
Block a user