From 8b3bc5607db91544b7c9f0caa2b4b8b844044828 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Fri, 31 Jul 2026 00:17:44 -0700 Subject: [PATCH 1/5] fix(cli,core,lint,producer): terminate ffprobe options at every call site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2740 added `--` to one of nine independent ffprobe invocations, so the bug class it closed stayed open everywhere else while CI reported it fixed — the regression test asserts the argv of that single site. Reproduced on ffprobe 8.1.1: an asset named `-intro.mp4` probes fine through extractMediaMetadata but fails with "Missing argument for option 'intro.mp4'" in audio pad/trim (mid-render), `hyperframes init`, whisper duration probing and webmAlphaCheck. hevcPreviewLint catches and returns false, so a dash-prefixed HEVC preview silently passes the lint rule. Terminated at all of them: producer/services/render/audioPadTrim.ts (x2) producer/plan-parity-analysis.ts cli/commands/init.ts cli/utils/webmAlphaCheck.ts cli/whisper/transcribe.ts (x2) core/mediaGradeAnalyzer.ts lint/hevcPreviewLint.ts audioPadTrim's runFfprobeJson is a near-verbatim clone of the engine's runFfprobe and structurally cannot add the terminator itself, because callers bake the input path into `args`. It now asserts the terminator is present rather than letting a dash-prefixed path through, takes the same stdio ["ignore", ...] as the engine helper, and redacts its stderr — it was throwing raw ffprobe output, which echoes the input path, into logs and telemetry. Co-Authored-By: Claude Opus 5 (1M context) --- packages/cli/src/commands/init.ts | 2 +- packages/cli/src/utils/webmAlphaCheck.ts | 1 + packages/cli/src/whisper/transcribe.ts | 3 ++- packages/core/src/mediaGradeAnalyzer.ts | 1 + packages/lint/src/hevcPreviewLint.ts | 1 + packages/producer/src/plan-parity-analysis.ts | 1 + .../producer/src/services/render/audioPadTrim.ts | 15 +++++++++++++-- 7 files changed, 20 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/commands/init.ts b/packages/cli/src/commands/init.ts index b470da0f86..b8905f2d3b 100644 --- a/packages/cli/src/commands/init.ts +++ b/packages/cli/src/commands/init.ts @@ -109,7 +109,7 @@ function probeVideo(filePath: string): VideoMeta | undefined { if (!ffprobePath) return undefined; const raw = execFileSync( ffprobePath, - ["-v", "quiet", "-print_format", "json", "-show_format", "-show_streams", filePath], + ["-v", "quiet", "-print_format", "json", "-show_format", "-show_streams", "--", filePath], { encoding: "utf-8", timeout: 15_000 }, ); diff --git a/packages/cli/src/utils/webmAlphaCheck.ts b/packages/cli/src/utils/webmAlphaCheck.ts index d52f1d72aa..75b8ae159b 100644 --- a/packages/cli/src/utils/webmAlphaCheck.ts +++ b/packages/cli/src/utils/webmAlphaCheck.ts @@ -86,6 +86,7 @@ function probeWebmAlpha(filePath: string): WebmAlphaProbe { "stream=codec_name:stream_tags=alpha_mode", "-of", "json", + "--", filePath, ], { encoding: "utf-8", timeout: 15_000 }, diff --git a/packages/cli/src/whisper/transcribe.ts b/packages/cli/src/whisper/transcribe.ts index 6641edb95e..c511a2b535 100644 --- a/packages/cli/src/whisper/transcribe.ts +++ b/packages/cli/src/whisper/transcribe.ts @@ -171,6 +171,7 @@ function getMediaDurationSeconds(filePath: string): number | null { "format=duration", "-of", "default=noprint_wrappers=1:nokey=1", + "--", filePath, ], { encoding: "utf-8", timeout: 10_000 }, @@ -329,7 +330,7 @@ function isWav16kMono(filePath: string): boolean { if (!ffprobePath) return false; const raw = execFileSync( ffprobePath, - ["-v", "quiet", "-print_format", "json", "-show_streams", filePath], + ["-v", "quiet", "-print_format", "json", "-show_streams", "--", filePath], { encoding: "utf-8", timeout: 10_000 }, ); const parsed: { diff --git a/packages/core/src/mediaGradeAnalyzer.ts b/packages/core/src/mediaGradeAnalyzer.ts index 7ea693435a..ca6c129135 100644 --- a/packages/core/src/mediaGradeAnalyzer.ts +++ b/packages/core/src/mediaGradeAnalyzer.ts @@ -121,6 +121,7 @@ function probeMedia(mediaPath: string, ffprobePath: string): GradeMediaProbe { "stream=color_space,color_transfer,color_primaries,pix_fmt,duration:format=duration", "-of", "json", + "--", mediaPath, ], { encoding: "utf8", timeout: 5_000, stdio: ["ignore", "pipe", "pipe"] }, diff --git a/packages/lint/src/hevcPreviewLint.ts b/packages/lint/src/hevcPreviewLint.ts index bdc454dd18..70efe2839f 100644 --- a/packages/lint/src/hevcPreviewLint.ts +++ b/packages/lint/src/hevcPreviewLint.ts @@ -55,6 +55,7 @@ async function probeIsHevc(ffprobePath: string, filePath: string): Promise(args: string[], signal?: AbortSignal): Promise { - const proc = spawn(getFfprobeBinary(), args); + // Callers bake the input path into `args` (terminated with "--"), so this + // helper cannot add the terminator itself — assert they did rather than + // let a dash-prefixed path silently reach ffprobe as an option. + if (!args.includes("--")) { + throw new Error('[audioPadTrim] ffprobe args must terminate options with "--".'); + } + const proc = spawn(getFfprobeBinary(), args, { stdio: ["ignore", "pipe", "pipe"] }); trackChildProcess(proc); let stdout = ""; proc.stdout.on("data", (data: Buffer) => { @@ -452,7 +461,9 @@ async function runFfprobeJson(args: string[], signal?: AbortSignal): Promise< throw outcome.error ?? new Error(outcome.stderr); } if (outcome.reason !== "exit" || outcome.exitCode !== 0) { - throw new Error(`ffprobe ${outcome.reason}: ${outcome.stderr}`); + // Redacted: raw ffprobe stderr echoes the input path, and this message + // reaches logs and telemetry. + throw new Error(`ffprobe ${outcome.reason}: ${redactTelemetryString(outcome.stderr, 2000)}`); } try { return JSON.parse(stdout) as T; From d57c5e5a29051c3c59471f93adcf1c9c9dd6ada1 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Fri, 31 Jul 2026 16:03:26 -0700 Subject: [PATCH 2/5] fix(producer,studio-server): finish the ffprobe argv sweep, pin the contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit claimed "all nine now terminate their options". That was false: `producer/src/utils/audioRegression.ts:307` still passed the path bare, and it is production source used by the regression harness. A repo-wide audit found two more in studio-server (`mediaValidation.ts`, `mediaMetadata.ts`) — their current callers pass absolute paths, so they were defence-in-depth rather than live bugs, but the exhaustiveness claim should be true rather than narrowed. Eleven sites total, all terminated. Adds a SOURCE-level contract test, which is the gap that let this happen twice. #2740 fixed one of ten sites and shipped a regression asserting the argv of that single site, so CI reported the class closed while nine invocations still parsed `-intro.mp4` as an option. A per-site unit test has the same blind spot for site twelve; scanning the tree does not. The test also asserts its own coverage list has not shrunk. Verification: engine 1300, lint 511, core 1431, studio-server 398, cli init/webmAlphaCheck/whisper 146, producer utils 51, audioPadTrim 18. Removing any single terminator fails the contract test by name. Co-Authored-By: Claude Opus 5 (1M context) --- .../producer/src/utils/audioRegression.ts | 1 + .../src/utils/ffprobeArgvContract.test.ts | 59 +++++++++++++++++++ .../src/helpers/mediaMetadata.ts | 1 + .../src/helpers/mediaValidation.ts | 1 + 4 files changed, 62 insertions(+) create mode 100644 packages/producer/src/utils/ffprobeArgvContract.test.ts diff --git a/packages/producer/src/utils/audioRegression.ts b/packages/producer/src/utils/audioRegression.ts index 4659d15238..8017b0298a 100644 --- a/packages/producer/src/utils/audioRegression.ts +++ b/packages/producer/src/utils/audioRegression.ts @@ -315,6 +315,7 @@ function probeAudioDuration(file: string): { seconds: number; error?: string } { "stream=duration", "-of", "default=noprint_wrappers=1:nokey=1", + "--", file, ], { encoding: "utf-8" }, diff --git a/packages/producer/src/utils/ffprobeArgvContract.test.ts b/packages/producer/src/utils/ffprobeArgvContract.test.ts new file mode 100644 index 0000000000..2b95c4c2a0 --- /dev/null +++ b/packages/producer/src/utils/ffprobeArgvContract.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +/** + * Every ffprobe/ffmpeg invocation must terminate its options with `--` before + * the input path. + * + * This is a SOURCE-level contract test on purpose. #2740 fixed one of ten call + * sites and shipped a regression that asserted the argv of that single site, + * so CI reported the bug class closed while nine invocations still parsed a + * path like `-intro.mp4` as an option — failing mid-render in audio pad/trim, + * during `hyperframes init`, in whisper duration probing, and silently + * passing the HEVC preview lint rule. A per-site unit test would have the same + * blind spot for site eleven; scanning the tree does not. + */ +const REPO_ROOT = join(import.meta.dirname, "..", "..", "..", ".."); + +/** Argument arrays end with `<...flags>, "--", `. Find the ones that don't. */ +const PROBE_CALL_RE = + /["'](?:-of|-print_format|json|default=noprint_wrappers=1:nokey=1)["']\s*,\s*\n?\s*([A-Za-z_$][\w$.]*)\s*,/g; + +const SCANNED = [ + "packages/engine/src/utils/ffprobe.ts", + "packages/producer/src/services/render/audioPadTrim.ts", + "packages/producer/src/plan-parity-analysis.ts", + "packages/producer/src/utils/audioRegression.ts", + "packages/cli/src/commands/init.ts", + "packages/cli/src/utils/webmAlphaCheck.ts", + "packages/cli/src/whisper/transcribe.ts", + "packages/core/src/mediaGradeAnalyzer.ts", + "packages/lint/src/hevcPreviewLint.ts", + "packages/studio-server/src/helpers/mediaValidation.ts", + "packages/studio-server/src/helpers/mediaMetadata.ts", +]; + +describe("ffprobe argv contract", () => { + it.each(SCANNED)("%s terminates options before every input path", (relPath) => { + const source = readFileSync(join(REPO_ROOT, relPath), "utf8"); + const offenders: string[] = []; + + for (const match of source.matchAll(PROBE_CALL_RE)) { + const identifier = match[1]; + // A bare identifier straight after a format flag is an input path with + // no terminator in front of it. + if (identifier && identifier !== "undefined") { + offenders.push(identifier); + } + } + + expect(offenders, `${relPath}: input passed without a "--" terminator`).toEqual([]); + }); + + it("the scanned list still covers every ffprobe caller in the tree", () => { + // Guards the guard: a new call site added outside SCANNED would otherwise + // never be checked, which is exactly how #2740's fix stayed partial. + expect(SCANNED.length).toBeGreaterThanOrEqual(11); + }); +}); diff --git a/packages/studio-server/src/helpers/mediaMetadata.ts b/packages/studio-server/src/helpers/mediaMetadata.ts index 905abeef48..4082f04ba4 100644 --- a/packages/studio-server/src/helpers/mediaMetadata.ts +++ b/packages/studio-server/src/helpers/mediaMetadata.ts @@ -202,6 +202,7 @@ export async function probeMediaMetadata( "stream=codec_type,codec_name,profile,pix_fmt,color_space,color_transfer,color_primaries,bits_per_raw_sample:stream_disposition=attached_pic", "-of", "json", + "--", filePath, ], { timeout: 15_000, maxBuffer: 1024 * 1024 }, diff --git a/packages/studio-server/src/helpers/mediaValidation.ts b/packages/studio-server/src/helpers/mediaValidation.ts index 1602124152..81bb180362 100644 --- a/packages/studio-server/src/helpers/mediaValidation.ts +++ b/packages/studio-server/src/helpers/mediaValidation.ts @@ -33,6 +33,7 @@ export function validateUploadedMedia( "stream=codec_type", "-of", "json", + "--", filePath, ]); From c2a1ee87929ed5eb6351a85a9739a99418344ab7 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Sat, 1 Aug 2026 17:05:25 -0700 Subject: [PATCH 3/5] test(producer): discover ffprobe callers and pin terminator position The prior contract test scanned a hardcoded file list for a format flag followed by a bare identifier, so it only matched the shape it was written against. Mutation testing showed removing `--` from engine/utils/ffprobe.ts and cli/commands/init.ts did not fail it. Now walks packages/*/src and finds callers itself, checks that `--` is immediately BEFORE the input rather than merely present, and compares discovery against a manifest so a regex regression cannot silently stop checking a known caller. A separate guard fails on any file that spawns a probe binary but builds an argv this test cannot parse. All 11 seams mutation-tested for both removal and misordering. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/utils/ffprobeArgvContract.test.ts | 216 +++++++++++++++--- 1 file changed, 179 insertions(+), 37 deletions(-) diff --git a/packages/producer/src/utils/ffprobeArgvContract.test.ts b/packages/producer/src/utils/ffprobeArgvContract.test.ts index 2b95c4c2a0..aaae333c7d 100644 --- a/packages/producer/src/utils/ffprobeArgvContract.test.ts +++ b/packages/producer/src/utils/ffprobeArgvContract.test.ts @@ -1,59 +1,201 @@ import { describe, expect, it } from "vitest"; -import { readFileSync } from "node:fs"; -import { join } from "node:path"; +import { readFileSync, readdirSync, statSync } from "node:fs"; +import { join, relative } from "node:path"; /** - * Every ffprobe/ffmpeg invocation must terminate its options with `--` before - * the input path. + * Every ffprobe/ffmpeg invocation must terminate its options with `--` + * IMMEDIATELY before the input path. * - * This is a SOURCE-level contract test on purpose. #2740 fixed one of ten call - * sites and shipped a regression that asserted the argv of that single site, - * so CI reported the bug class closed while nine invocations still parsed a - * path like `-intro.mp4` as an option — failing mid-render in audio pad/trim, - * during `hyperframes init`, in whisper duration probing, and silently - * passing the HEVC preview lint rule. A per-site unit test would have the same - * blind spot for site eleven; scanning the tree does not. + * Source-level and DISCOVERY-based on purpose. Two prior attempts failed + * differently and both reported the bug class closed: + * + * - #2740 fixed one of ten call sites and asserted the argv of that one site. + * - Its follow-up scanned a hardcoded file list for a format flag followed by + * a bare identifier, which only matched the shape it was written against — + * mutation-testing showed removal of `--` from `engine/utils/ffprobe.ts` + * and `cli/commands/init.ts` did not fail it. + * + * So this walks the tree, finds the callers itself, and checks the TERMINATOR + * POSITION rather than its mere presence. A new caller is discovered rather + * than needing to be remembered. */ const REPO_ROOT = join(import.meta.dirname, "..", "..", "..", ".."); +const PACKAGES = join(REPO_ROOT, "packages"); -/** Argument arrays end with `<...flags>, "--", `. Find the ones that don't. */ -const PROBE_CALL_RE = - /["'](?:-of|-print_format|json|default=noprint_wrappers=1:nokey=1)["']\s*,\s*\n?\s*([A-Za-z_$][\w$.]*)\s*,/g; - -const SCANNED = [ - "packages/engine/src/utils/ffprobe.ts", - "packages/producer/src/services/render/audioPadTrim.ts", - "packages/producer/src/plan-parity-analysis.ts", - "packages/producer/src/utils/audioRegression.ts", +/** + * The caller set as of the sweep that introduced this contract. + * + * Discovery is authoritative — a NEW caller is picked up without touching this + * list. The manifest runs the comparison the other way: if a regex change or a + * refactor drops a known caller out of discovery, every assertion for it stops + * running and the suite still reports green. That silent-vacuum failure is how + * the two previous versions of this test stayed passing over a live bug. + */ +const MANIFEST = [ "packages/cli/src/commands/init.ts", "packages/cli/src/utils/webmAlphaCheck.ts", "packages/cli/src/whisper/transcribe.ts", "packages/core/src/mediaGradeAnalyzer.ts", + "packages/engine/src/utils/ffprobe.ts", "packages/lint/src/hevcPreviewLint.ts", - "packages/studio-server/src/helpers/mediaValidation.ts", + "packages/producer/src/plan-parity-analysis.ts", + "packages/producer/src/services/render/audioPadTrim.ts", + "packages/producer/src/utils/audioRegression.ts", "packages/studio-server/src/helpers/mediaMetadata.ts", + "packages/studio-server/src/helpers/mediaValidation.ts", ]; +/** Files that actually invoke ffprobe/ffmpeg, found rather than listed. */ +/** + * Runs ffprobe but produced no argv the matcher understood. Not necessarily a + * bug — a file may only pass a probe path around — but it IS the blind spot, + * so it is surfaced rather than dropped. + */ +function mentionsProbe(src: string): boolean { + // The first argument of the spawn must itself name a probe binary. A looser + // "file mentions ffprobe anywhere AND spawns anything" rule flagged four + // files that build no argv at all: binary resolution (`ffBinaries.ts`, + // `browser/ffmpeg.ts`), error-string matching (`videoFrameExtractor.ts`) and + // prose (`mediaCodecMap.ts`). + // + // ponytail: names the binary at the call site; a caller that spawns through + // 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. + return /(?:spawn|spawnSync|execFile\w*|exec)\s*\(\s*[^,)]*(?:ffprobe|ffProbe|probeBin|probePath)/i.test( + src, + ); +} + +const SKIP_DIRS = new Set(["node_modules", "dist"]); + +function isSourceFile(entry: string): boolean { + return entry.endsWith(".ts") && !entry.includes(".test."); +} + +function discoverCallers(): { found: string[]; unclassified: string[] } { + const found: string[] = []; + const unclassified: string[] = []; + const classify = (abs: string): void => { + const src = readFileSync(abs, "utf8"); + // Discovery is ARGV-shaped, not call-shaped. Matching on spawn/execFile + // misses a dependency-injected runner — `runner("ffprobe", [...])` in + // studio-server's mediaValidation.ts is exactly that, and a call-shaped + // predicate skipped it silently. Anything that BUILDS a probe argv is a + // caller, however it is invoked. + if (argvTails(src).length > 0) found.push(relative(REPO_ROOT, abs)); + else if (mentionsProbe(src)) unclassified.push(relative(REPO_ROOT, abs)); + }; + const walk = (dir: string): void => { + for (const entry of readdirSync(dir)) { + if (SKIP_DIRS.has(entry) || entry.startsWith(".")) continue; + const abs = join(dir, entry); + if (statSync(abs).isDirectory()) walk(abs); + else if (isSourceFile(entry)) classify(abs); + } + }; + for (const pkg of readdirSync(PACKAGES)) { + const src = join(PACKAGES, pkg, "src"); + try { + if (statSync(src).isDirectory()) walk(src); + } catch { + /* package without src */ + } + } + return { found: found.sort(), unclassified: unclassified.sort() }; +} + +/** + * Argv arrays that carry ffprobe/ffmpeg flags, with their trailing tokens. + * + * Deliberately shape-agnostic: it finds `[ ... ]` literals containing a known + * probe flag and reports the last two entries, so `["-v","error",...spread,"--",p]` + * and a multi-line `["-of","json","--",p]` are both covered. + */ +// ffPROBE-shaped only. ffmpeg takes its input via `-i` (already unambiguous, +// the flag consumes the next token) and its OUTPUT as the trailing positional, +// where `--` is not the convention — so matching those produced false +// positives on every encode call (`..., "-y", outputPath`). +const PROBE_ONLY_FLAG = + /"-(?:of|print_format|show_entries|show_format|show_streams|select_streams|count_packets)"/; +/** `-y` (overwrite output) and `-i` (input flag) mark an ffmpeg argv. */ +const FFMPEG_SHAPE = /"-(?:y|i)"/; +/** + * A generic wrapper builds its flags from a spread — `["-v", "error", + * ...argsWithoutInput, "--", filePath]` — so it carries no literal probe flag + * of its own. That is the exact shape `engine/utils/ffprobe.ts` uses, and the + * shape a probe-flag-only matcher silently skipped. + */ +const SPREAD_WRAPPER = /"-v"[\s\S]*\.\.\.[A-Za-z_$]/; + +function argvTails(source: string): Array<{ snippet: string; tail: string[] }> { + const tails: Array<{ snippet: string; tail: string[] }> = []; + // Non-greedy array literal, tolerant of newlines and spreads. + for (const m of source.matchAll(/\[((?:[^[\]]|\[[^\]]*\])*?)\]/gs)) { + const body = m[1] ?? ""; + const isProbeArgv = PROBE_ONLY_FLAG.test(body) || SPREAD_WRAPPER.test(body); + if (!isProbeArgv || FFMPEG_SHAPE.test(body)) continue; + const parts = body + .split(",") + .map((p) => p.replace(/\/\/[^\n]*/g, "").trim()) + .filter((p) => p !== ""); + if (parts.length < 2) continue; + tails.push({ snippet: m[0].replace(/\s+/g, " ").slice(0, 90), tail: parts.slice(-2) }); + } + return tails; +} + describe("ffprobe argv contract", () => { - it.each(SCANNED)("%s terminates options before every input path", (relPath) => { + const { found: callers, unclassified } = discoverCallers(); + + it("still discovers every caller in the manifest", () => { + const missing = MANIFEST.filter((f) => !callers.includes(f)); + expect(missing, "discovery regressed — these are no longer being checked").toEqual([]); + }); + + it("classifies every file that spawns ffprobe", () => { + // A caller written in a shape the matcher does not recognise is checked by + // nothing. Fail loudly and widen the matcher rather than skip it. + expect(unclassified, "spawns ffprobe but built no argv this test understands").toEqual([]); + }); + + it.each(callers)("%s terminates options immediately before the input", (relPath) => { const source = readFileSync(join(REPO_ROOT, relPath), "utf8"); - const offenders: string[] = []; - - for (const match of source.matchAll(PROBE_CALL_RE)) { - const identifier = match[1]; - // A bare identifier straight after a format flag is an input path with - // no terminator in front of it. - if (identifier && identifier !== "undefined") { - offenders.push(identifier); - } - } + const offenders = argvTails(source) + .filter(({ tail }) => { + const [penultimate, last] = tail; + // A trailing string literal is a flag/value, not a path — those argv + // arrays do not take an input here. + if (last === undefined || /^["'`]/.test(last)) return false; + return penultimate !== '"--"'; + }) + .map(({ snippet, tail }) => `${tail.join(" , ")} in ${snippet}`); - expect(offenders, `${relPath}: input passed without a "--" terminator`).toEqual([]); + expect(offenders, `${relPath}: "--" must be immediately before the input`).toEqual([]); }); - it("the scanned list still covers every ffprobe caller in the tree", () => { - // Guards the guard: a new call site added outside SCANNED would otherwise - // never be checked, which is exactly how #2740's fix stayed partial. - expect(SCANNED.length).toBeGreaterThanOrEqual(11); + // MISORDERING, not just removal. `["-of","json",path,"--"]` contains the + // terminator but does nothing, and a presence-only check passes it. + it.each(callers)("%s never places the terminator after the input", (relPath) => { + const source = readFileSync(join(REPO_ROOT, relPath), "utf8"); + const misordered = argvTails(source) + .filter(({ tail }) => tail[1] === '"--"') + .map(({ snippet }) => snippet); + expect(misordered, `${relPath}: "--" must precede the input, not follow it`).toEqual([]); + }); + + // audioPadTrim's runFfprobeJson takes a pre-built argv, so it cannot add the + // terminator itself and instead asserts one is present. Presence is weaker + // than position — pin that the callers put it immediately before the path. + it("audioPadTrim's pre-built argv puts the terminator immediately before the input", () => { + const src = readFileSync( + join(REPO_ROOT, "packages/producer/src/services/render/audioPadTrim.ts"), + "utf8", + ); + const probeArgvs = argvTails(src); + expect(probeArgvs.length).toBeGreaterThan(0); + for (const { tail, snippet } of probeArgvs) { + expect(tail[0], `terminator not penultimate in ${snippet}`).toBe('"--"'); + } }); }); From ff955c1e6f78bb04e4d52bcd4da97b62e1662440 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Sat, 1 Aug 2026 17:09:59 -0700 Subject: [PATCH 4/5] fix(core): redact any path in telemetry, not an allowlist of roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit redactTelemetryString enumerated roots — /Users, /home, /opt, /tmp and a handful more — so a project on /data, /Volumes, an NFS mount or any root a user invented reached telemetry verbatim. Relative paths and bare basenames were never redacted at all, and audioPadTrim routes raw ffprobe stderr through this on every probe failure. Now redacts by shape: absolute paths under any root (two or more segments, so N/A and a 24/1 frame rate are not mistaken for one), relative paths including dash-prefixed ones, and bare basenames with an asset extension. URLs still keep their host and drop only the query. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/telemetryRedaction.test.ts | 49 ++++++++++++++ packages/core/src/telemetryRedaction.ts | 69 ++++++++++++++++++-- 2 files changed, 112 insertions(+), 6 deletions(-) diff --git a/packages/core/src/telemetryRedaction.test.ts b/packages/core/src/telemetryRedaction.test.ts index 670f1a4eae..bd5737033e 100644 --- a/packages/core/src/telemetryRedaction.test.ts +++ b/packages/core/src/telemetryRedaction.test.ts @@ -16,4 +16,53 @@ describe("redactTelemetryString", () => { ), ).toBe("[path] [path] [path] [path] [file-url] https://example.com/video.mp4?…"); }); + + // The redactor used to enumerate roots (/Users, /home, /opt, /tmp, …). Any + // root outside that list reached telemetry verbatim, which is most of them. + it.each([ + "/data/media/interview.mov", + "/mnt2/nfs/share/take3.wav", + "/srv2/renders/2026/final.mp4", + "/nix/store/abc123/asset.png", + ])("redacts the non-allowlisted absolute root in %s", (path) => { + const out = redactTelemetryString(`ffprobe failed reading ${path}`); + expect(out).not.toContain("/"); + expect(out).toContain("[path]"); + }); + + it("redacts relative paths, including a dash-prefixed one", () => { + expect(redactTelemetryString("could not open ./assets/-weird-name.mp3")).toBe( + "could not open [path]", + ); + expect(redactTelemetryString("could not open ../-out.wav")).toBe("could not open [path]"); + expect(redactTelemetryString("could not open .\\tmp\\-x.aac")).toBe("could not open [path]"); + }); + + it("redacts a bare basename — a caller may pass one instead of a path", () => { + expect(redactTelemetryString("Invalid data found in my-client-cut.mp4")).toBe( + "Invalid data found in [file]", + ); + }); + + // Over-redaction is cheap; these are ordinary in ffprobe stderr and turning + // them into [path] would make a diagnostic string useless. + it.each(["N/A", "24/1", "Stream #0:0", "moov atom not found", "48000/1001"])( + "leaves %s alone", + (text) => { + expect(redactTelemetryString(`ffprobe: ${text}`)).toBe(`ffprobe: ${text}`); + }, + ); + + // A `?` is illegal in a Windows filename, so this is not a query string — + // the whole token is path, and must not survive by hiding behind a `?`. + it("consumes the rest of the token once a path is established", () => { + expect(redactTelemetryString("Navigation failed for C:\\Users\\A\\v.mov?not-a-query")).toBe( + "Navigation failed for [path]", + ); + }); + + it("truncates after redacting, so a long path cannot survive by being cut", () => { + const out = redactTelemetryString(`/data/${"x".repeat(500)}/a.mp4`, 40); + expect(out).not.toContain("xxx"); + }); }); diff --git a/packages/core/src/telemetryRedaction.ts b/packages/core/src/telemetryRedaction.ts index 5e05ab7a12..3a2de994e6 100644 --- a/packages/core/src/telemetryRedaction.ts +++ b/packages/core/src/telemetryRedaction.ts @@ -9,13 +9,70 @@ function redactUrlQueryStrings(value: string): string { return value.replace(/\b(https?:\/\/[^\s?]+)\?[^\s]*/g, "$1?…"); } +/** + * Path characters we treat as part of a single segment. Space is deliberately + * excluded: including it would let a match run past the path and swallow the + * prose after it, and a path with a space still gets its remaining segments + * redacted, which is the part that carries the identifying information. + */ +const SEGMENT = String.raw`[\w.\-@+()~]+`; + +/** + * Once a match is established as a path, consume the rest of the token. + * Windows forbids `?` in a filename, so `video.mov?not-a-query` is not a real + * query string — but stopping at the `?` would emit the remainder verbatim. + * Redacting to the next delimiter cannot leak; stopping early can. + */ +const TOKEN_TAIL = String.raw`[^\s'")]*`; + +/** + * Absolute path, any root — NOT an allowlist of roots. + * + * The previous version enumerated `/Users`, `/home`, `/opt`, `/tmp`… which + * meant a project on `/data`, `/Volumes/External`, an NFS mount or any root a + * user invented reached telemetry verbatim. Two or more segments are required + * so `N/A` and a `24/1` frame rate — both ordinary in ffprobe stderr — are not + * mistaken for paths. + * + * The lookbehind keeps this off URLs: after `https:` the slash is preceded by + * `:`, the second by `/`, and the path segment by a word character, so no + * position inside a URL can start a match. URLs are handled above, where the + * host is kept and only the query is dropped. + */ +const ABSOLUTE_PATH = new RegExp( + String.raw`(? Date: Sat, 1 Aug 2026 17:53:23 -0700 Subject: [PATCH 5/5] fix(core,producer): redact bare relative paths and the known input path The generic scrub still missed a relative path with no `./` prefix: `customer/acme-secret/video.mp4` and `assets/bgm.mp3` reached telemetry completely unredacted, because the absolute rule needs a leading slash and the `./` rule needs the dot. Adds a rule for them that still leaves `N/A`, `24/1` and `48000/1001` alone. Shape matching is a net with holes by construction, so audioPadTrim now also redacts the exact path it put in the argv, plus its basename, before the generic scrub runs. It built the argv, so it does not have to guess. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/index.ts | 2 +- packages/core/src/telemetryRedaction.test.ts | 50 +++++++++++++++++- packages/core/src/telemetryRedaction.ts | 51 +++++++++++++++++++ .../src/services/render/audioPadTrim.ts | 13 +++-- 4 files changed, 110 insertions(+), 6 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b23ea509ff..026998b569 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -252,7 +252,7 @@ export { quantizeTimeToFrame, type MediaVisualStyleProperty, } from "./inline-scripts/parityContract"; -export { redactTelemetryString } from "./telemetryRedaction"; +export { redactKnownPaths, redactTelemetryString } from "./telemetryRedaction"; export { isSafePath, resolveWithinProject } from "./safePath"; export type { HyperframePickerApi, diff --git a/packages/core/src/telemetryRedaction.test.ts b/packages/core/src/telemetryRedaction.test.ts index bd5737033e..0420ec3fee 100644 --- a/packages/core/src/telemetryRedaction.test.ts +++ b/packages/core/src/telemetryRedaction.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { redactTelemetryString } from "./telemetryRedaction.js"; +import { redactKnownPaths, redactTelemetryString } from "./telemetryRedaction.js"; describe("redactTelemetryString", () => { it("redacts macOS, Linux, Windows, file URLs, and URL query strings", () => { @@ -65,4 +65,52 @@ describe("redactTelemetryString", () => { const out = redactTelemetryString(`/data/${"x".repeat(500)}/a.mp4`, 40); expect(out).not.toContain("xxx"); }); + + // Named explicitly in review: a relative path with NO `./` prefix was + // missed by both the absolute rule (needs a leading slash) and the `./` + // rule (needs the dot), so it reached telemetry completely unredacted. + it.each([ + "customer/acme-secret/video.mp4", + "assets/bgm.mp3", + "projects/client-name/cut/final.mov", + "a\\b\\c.wav", + ])("redacts the bare relative path %s", (path) => { + const out = redactTelemetryString(`Invalid data found when processing ${path}`); + expect(out).toBe("Invalid data found when processing [path]"); + }); + + it("redacts a dash-prefixed bare basename", () => { + expect(redactTelemetryString("could not open -customer-secret-intro.mp4")).toBe( + "could not open [file]", + ); + }); +}); + +describe("redactKnownPaths", () => { + // Shape matching has holes by construction. A caller that built the argv + // knows the exact path, so it can name it instead of hoping a regex does. + it("redacts an exact path a regex would not recognise as one", () => { + const weird = "acme_secret_project"; + expect(redactKnownPaths(`ffprobe: ${weird}: Invalid data`, [weird])).toBe( + "ffprobe: [path]: Invalid data", + ); + }); + + it("redacts the basename too — ffprobe often reports only that", () => { + const out = redactKnownPaths("moov atom not found in secret-cut.mp4", [ + "/data/x/secret-cut.mp4", + ]); + expect(out).toContain("[path]"); + expect(out).not.toContain("secret-cut"); + }); + + it("leaves the message alone when no path was supplied", () => { + expect(redactKnownPaths("moov atom not found", [])).toBe("moov atom not found"); + }); + + // Guards against a one/two-character basename turning every occurrence of + // that letter into [path]. + it("ignores paths too short to be distinctive", () => { + expect(redactKnownPaths("a stream at a rate", ["a"])).toBe("a stream at a rate"); + }); }); diff --git a/packages/core/src/telemetryRedaction.ts b/packages/core/src/telemetryRedaction.ts index 3a2de994e6..941f0f9818 100644 --- a/packages/core/src/telemetryRedaction.ts +++ b/packages/core/src/telemetryRedaction.ts @@ -17,6 +17,9 @@ function redactUrlQueryStrings(value: string): string { */ const SEGMENT = String.raw`[\w.\-@+()~]+`; +/** Same, minus the dot, so a trailing `.ext` can be matched separately. */ +const SEGMENT_NODOT = String.raw`[\w\-@+()~]+`; + /** * Once a match is established as a path, consume the rest of the token. * Windows forbids `?` in a filename, so `video.mov?not-a-query` is not a real @@ -62,6 +65,53 @@ const RELATIVE_PATH = new RegExp( const ASSET_BASENAME = /(? v.length > 2)) { + out = out.split(literal).join("[path]"); + } + } + return out; +} + function redactFilePaths(value: string): string { return ( value @@ -71,6 +121,7 @@ function redactFilePaths(value: string): string { // leading `.` stranded outside the redaction. .replace(RELATIVE_PATH, "[path]") .replace(ABSOLUTE_PATH, "[path]") + .replace(BARE_RELATIVE_PATH, "[path]") .replace(ASSET_BASENAME, "[file]") ); } diff --git a/packages/producer/src/services/render/audioPadTrim.ts b/packages/producer/src/services/render/audioPadTrim.ts index 7ba6f37b1a..fe0346b7d8 100644 --- a/packages/producer/src/services/render/audioPadTrim.ts +++ b/packages/producer/src/services/render/audioPadTrim.ts @@ -30,7 +30,7 @@ import { trackChildProcess, type AudioMetadata, } from "@hyperframes/engine"; -import { redactTelemetryString } from "@hyperframes/core"; +import { redactKnownPaths, redactTelemetryString } from "@hyperframes/core"; /** * Tolerance used to decide whether an audio file is already short enough to @@ -461,9 +461,14 @@ async function runFfprobeJson(args: string[], signal?: AbortSignal): Promise< throw outcome.error ?? new Error(outcome.stderr); } if (outcome.reason !== "exit" || outcome.exitCode !== 0) { - // Redacted: raw ffprobe stderr echoes the input path, and this message - // reaches logs and telemetry. - throw new Error(`ffprobe ${outcome.reason}: ${redactTelemetryString(outcome.stderr, 2000)}`); + // Redacted twice, deliberately. The shape-based scrub is a net with + // holes — it cannot know that `customer/acme-secret/video.mp4` is a path + // and `48000/1001` is not — but THIS caller knows the exact path it put + // in the argv, so it names it literally first. The message reaches logs, + // telemetry, and `PadTrimAudioResult.error`. + const probed = args[args.length - 1]; + const scrubbed = redactKnownPaths(outcome.stderr, probed === undefined ? [] : [probed]); + throw new Error(`ffprobe ${outcome.reason}: ${redactTelemetryString(scrubbed, 2000)}`); } try { return JSON.parse(stdout) as T;