fix(cli,core,lint,producer,studio-server): terminate ffprobe options everywhere, pin the contract

[vanceingalls]

Replaces #2917, which was closed unmerged. Same content, re-verified. Independent of #2916 — rebased directly onto main.

Completes #2740, and adds the thing that stops this bug class coming back a third time.

The gap

#2740 added -- to one of ten independent ffprobe invocations. Its regression test asserted the argv of that single site, so CI reported the class closed while nine invocations still parsed a path like -intro.mp4 as an option.

Reproduced on ffprobe 8.1.1:

call site	failure
producer/services/render/audioPadTrim.ts ×2	Missing argument for option 'intro.mp4' — mid-render
cli/commands/init.ts	same, during hyperframes init
cli/whisper/transcribe.ts ×2	same, during duration probing
cli/utils/webmAlphaCheck.ts	same
core/mediaGradeAnalyzer.ts	same
producer/plan-parity-analysis.ts	same
lint/hevcPreviewLint.ts	catches and returns false — a dash-prefixed HEVC preview silently passes the rule
producer/utils/audioRegression.ts	the tenth, caught in review after I claimed "all nine"
studio-server/mediaValidation.ts, mediaMetadata.ts	defence-in-depth — current callers pass absolute paths

Eleven sites, all terminated.

The part that matters

ffprobeArgvContract.test.ts. A per-site unit test has exactly the blind spot
that let this happen twice — #2740 asserted its one site, and my own sweep then
missed a site and shipped no argv test at all.

The first version of this test had the same disease in a new shape: it scanned a
hardcoded file list for a format flag followed by a bare identifier, so it
only matched the shape it was written against. Review was right to reject it —
mutation testing showed that removing -- from engine/utils/ffprobe.ts,
cli/commands/init.ts and cli/whisper/transcribe.ts did not fail it.

What it does now:

• Discovers callers by walking packages/*/src rather than trusting a list,
  and discovery is argv-shaped, not call-shaped. A call-shaped predicate
  (spawn/execFile) missed studio-server/mediaValidation.ts, which invokes
  through an injected runner("ffprobe", …).
• Checks position, not presence. ["-of","json",path,"--"] contains the
  terminator and does nothing; a presence check passes it.
• Compares discovery against a manifest. If a regex change drops a known
  caller, its assertions stop running and the suite still goes green — the exact
  silent-vacuum failure the two previous versions had.
• Fails on anything it cannot classify. A file that spawns a probe binary but
  builds an argv the matcher doesn't understand is reported, not skipped.

Mutation-tested at all 11 seams, for both removal and misordering. Killing
argvTails outright fails the manifest test, so the guard is itself guarded.

The clone that couldn't take the fix

audioPadTrim.ts's runFfprobeJson is a near-verbatim copy of the engine's
runFfprobe and structurally cannot add -- itself, because callers bake the
input path into args. It now asserts the terminator is present, 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,
telemetry, and PadTrimAudioResult.error.

Collapsing that duplication into the engine helper is worth doing, but not here:
the two have diverged in error-message prefix and caller contract.

The redaction was not actually redacting

redactTelemetryString enumerated roots — /Users, /home, /opt, /tmp and
a few more. Everything else went out verbatim: a project on /data or
/Volumes, any NFS mount, every relative path, and every bare basename. Review
demonstrated customer/acme-secret/video.mp4 surviving untouched, and it was
right.

Two changes, because one is not enough:

• By shape — absolute paths under any root, ./-prefixed and bare relative
  paths, and basenames with an asset extension. Two separators (or one plus an
  extension) are required, so N/A, a 24/1 frame rate and 48000/1001 still
  read normally. URLs keep their host and lose only the query.
• By literal — redactKnownPaths takes the exact path the caller put in the
  argv, plus its basename, and removes it before any pattern runs. Shape matching
  is a net with holes by construction; a caller that built the argv doesn't have
  to guess.

Verification

Full suites green on every touched package: engine 1299, cli 2325, core 1453,
lint 511, producer 491, studio-server 398. Typecheck and lint clean across all
six.

Every fix here has a regression that fails when that fix alone is reverted —
verified individually, including the contract test's own discovery guard.

🤖 Generated with Claude Code

[miguel-heygen]

Exact-head review: the production argv edits are mechanically correct at every current ffprobe seam, #2917’s missing callers are included, targeted contract tests pass 12/12, and CI is green. I am holding the stamp for two P2s:

1. packages/producer/src/utils/ffprobeArgvContract.test.ts:20 does not actually pin the claimed contract. Its regex misses removal of -- from three current production seams (packages/engine/src/utils/ffprobe.ts:52, packages/cli/src/commands/init.ts:112, packages/cli/src/whisper/transcribe.ts:333). The coverage assertion only checks SCANNED.length >= 11, so a new/unlisted caller is invisible, and arbitrary "json", identifier, source can false-positive. Please discover actual ffprobe call/argv expressions, compare the discovered caller set to the manifest, and mutation-test removal/misordering at every supported shape. At minimum, audioPadTrim should validate -- is penultimate rather than merely present.

2. packages/producer/src/services/render/audioPadTrim.ts:466 still exposes many exact input paths through the new error path. redactTelemetryString does not cover relative paths or arbitrary absolute roots. Real ffprobe stderr for -customer-secret-intro.mp4 is returned unchanged, as are /data/acme-secret/video.mp4 and customer/acme-secret/video.mp4, and that string propagates via PadTrimAudioResult.error. Please redact the exact validated input path and basename before the generic scrub, with tests for a dash-prefixed relative path and a non-allowlisted absolute root.

The runtime -- placements themselves are clean; these findings are about making the contract durable and the newly described redaction truthful.

[miguel-heygen]

Exact-head re-review at 30facd6072796dc962054e690c7eca720dd8005c.

The two R1 mechanisms are substantially improved: all current package/src TypeScript seams now fail removal/misordering mutations, and audioPadTrim exact-redacts the argv input before generic scrubbing. The focused contract/redaction suites pass 48/48. I am still holding the stamp for two P2s:

P2 — the “discovery” contract excludes shipped runtime callers where the same bug is still live. ffprobeArgvContract.test.ts:23-24,68-105 only walks packages/*/src, only .ts, and excludes tests. It therefore cannot discover .mjs/.cjs/shell skill code. Concrete current user-path calls still missing -- immediately before input include:

• skills/figma/scripts/verify-motion.mjs:47-55,63-73 (reference / render)
• skills/motion-graphics/grounding/locate.mjs:39-49 (img)
• skills/{faceless-explainer,pr-to-video,product-launch-video}/scripts/assemble-index.mjs:82-86 (abs)
• skills/media-use/scripts/lib/probe.mjs:13-16, audio/scripts/lib/tts.mjs:109-114, dither.mjs:120-124, and transcript-cut.mjs:202-213

I reproduced the same ffprobe failure here: a final -customer-secret-intro.mp4 without the terminator is parsed as an option and returns Missing argument for option 'customer-secret-intro.mp4'. These are distributed agent tools, not fixtures. Either sweep/discover the shipped skill callers too, or narrow the “every invocation / everywhere / bug class cannot recur” contract and land an explicit immediately-linked skill follow-up; as written the new green test gives repo-wide false confidence while existing user-facing paths remain broken.

P2 — the exact-path redaction is wired but not regression-tested at the caller, and a sibling error still returns the raw path. Every new assertion is against redactTelemetryString / redactKnownPaths in isolation. Removing audioPadTrim.ts:469-470 leaves all of them green; no test drives runFfprobeJson with real stderr and asserts the public PadTrimAudioResult.error omits the dash-prefixed/relative/arbitrary-root input. That is the exact wiring R1 asked to pin. Also defaultProbeVideoFrameInfo still throws ffprobe found no video stream in ${videoPath} at audioPadTrim.ts:392, which flows through failResult at :238-245 without the known-path scrub. Add one public-path regression and route all probe failures through the same known-input sanitizer.

CI note: package Test/Build/Lint/Typecheck and focused local tests are green, but exact-head Tests on windows-latest is currently red on an unrelated Studio 50k-fixture 5s timeout; regression shards 1-8 are still pending. The Windows failure does not explain either finding, but the gate is not terminal green yet.

[vanceingalls]

New head ce8b92966. Both R2 P2s addressed.

P2-1 — discovery excluded shipped skill callers. Confirmed and worse than the list: sweeping found 19 sites missing the terminator, not 9. Beyond the ones you named, skills/embedded-captions/scripts/{make-composition,make-cinematic,matte}.cjs, skills/media-use/scripts/{resolve,lib/grade-analyzer,lib/tts-local-provider}, and three package test files the old .test. exclusion was hiding (assemble.test.ts ×2, audioPadTrim.integration.test.ts, webm-concat-copy.test.ts ×3).

The sweep now covers packages/, skills/ and scripts/, including .mjs/.cjs and test files — dither.test.mjs was itself one of the broken sites. The only exclusion left is the contract test itself, which documents the contract with example argvs including a deliberately misordered one; scanning itself reported its own prose.

Two guard bugs surfaced while widening, both caught before they shipped:

• A blind insertion pass put -- after -i, which consumes the next token — it hit an ffmpeg input (spawnSync("ffmpeg", ["-i", absPath])) and a base64 -i, and would have broken both. Reverted; insertion is now driven by the same detector that reports offenders, not a regex, and I audited every added -- back to the binary it targets (19/19 ffprobe).
• mentionsProbe matched comment prose describing a spawn, flagging tts.test.mjs. Comments are stripped before the check.

Mutation-tested: removing -- from probe.mjs, verify-motion.mjs, matte.cjs, grade-analyzer.mjs and webm-concat-copy.test.ts each fail the suite.

P2-2 — redaction wired but not pinned at the caller. Your check reproduced exactly: deleting the audioPadTrim wiring left every redaction unit test green. Rather than patch the one thrower, all probe failures now go through a single sanitizeProbeFailure at the boundary where they enter PadTrimAudioResult.error — so defaultProbeVideoFrameInfo's no video stream in ${videoPath} is covered, and so is anything an injected probeVideoFrameInfo/probeAudioInfo throws. Per-thrower sanitizing is a list that drifts; the boundary can't be bypassed by adding a throw upstream.

Four public-path regressions drive padOrTrimAudioToVideoFrameCount and assert on PadTrimAudioResult.error for a dash-prefixed relative path, a non-allowlisted absolute root, a bare relative path, and raw stderr via the audio probe. All four fail with the wiring removed — verified by reverting it.

Suites: core 1453, producer 529, cli 2325, lint 511, studio-server 398, engine 1299 — all green. Lint and format clean on the 21 changed files.

Re-requesting review.

[vanceingalls]

Follow-up on 6c5403f7c — three things surfaced after the first push, all now fixed.

Rebased onto main (was 109 behind). CI tests the PR merged with main, which is how it saw a caller my branch predated: spawnSync("ffprobe", ["-version"]) in engine/src/utils/ffprobe.test.ts. That is a capability check with no runtime path, so there is nothing to terminate, but the unclassified guard flagged it. An all-literal argv now counts as taking no input; an argv with a bare identifier still has to be understood, verified by adding one and watching the guard fail.

CodeQL caught a ReDoS I introduced (js/redos) in the first version of that check. It was right: (?:"[^"]*"\s*,?\s*)+ nests a quantifier inside a quantifier with an optional separator, so whitespace matches two ways and a long non-matching argv backtracks exponentially. Replaced with a linear scan — find the spawn head, slice to the closing bracket, check the entries. Pathological input (4,000 repeated entries) now completes in 0.13 ms.

Pre-existing failures, for the record. After rebasing, core/cli/lint/studio-server show 3/5/2/1 failures. I baselined the same suites on clean origin/main and the counts are identical, so they are not from this branch. The earlier Tests on windows-latest red and an SDK install failure were both transient — the latter was literally 1579 packages installed / Failed to install 1 package, a registry fetch, not a lockfile mismatch (--frozen-lockfile is clean locally). Re-ran; green.

Current head is green on everything except the regression shards, still running.

Suites on this head: producer 594, engine 1370, and the producer:test:unit lane CI runs exits 0.

[miguel-heygen]

Reviewed at 6c5403f7. The JavaScript/TypeScript callers from R2 are fixed, caller-level audioPadTrim redaction is now wired, and the ReDoS alert is correctly removed with a linear scan. I am still requesting changes for three concrete issues:

1. [P2] The "whole-tree" ffprobe contract still excludes shipped shell/Python callers. ffprobeArgvContract.test.ts:32-35,132-160 walks skills/ but only admits .ts/.js/.mjs/.cjs. skills/remotion-to-hyperframes/scripts/frame_strip.sh:52-57 embeds Python and passes the user-controlled baseline directly as ffprobe's last positional argument without "--". A file named -client.mp4 is parsed as an option: locally the current argv fails with Missing argument for option 'client.mp4'; inserting -- makes ffprobe treat it as the input. The same scan also ignores four live ffprobe calls in skills/embedded-captions/scripts/render-and-composite.sh:355-381. Those happen to canonicalize their paths first, so they are not immediately exploitable, but they disprove the "everywhere"/discovery guarantee. Either include the runtime shell/Python surfaces in the gate or narrow the claim and add a separate contract for them; frame_strip.sh itself needs the terminator.

2. [P2] The new boundary sanitizer throws when an injected probe rejects a non-Error value. The comment at audioPadTrim.ts:223-227 correctly says caller-supplied probes can throw anything, but lines 263-276 cast the rejection to Error and pass .message into redactKnownPaths. For Promise.reject("probe failed"), that value is undefined; redactKnownPaths then throws at its out.split(...) call. Before this change the function returned a failed PadTrimAudioResult (with an ugly undefined message) rather than rejecting. Normalize first: reason instanceof Error ? reason.message : String(reason), and pin both injected probe seams with a non-Error rejection.

3. [P2/privacy] The new "any absolute root" redactor still emits Unicode paths verbatim. telemetryRedaction.ts:18-21,45-47,65-66 builds path segments from ASCII \w. Exact current-head repro: redactTelemetryString("ffprobe failed reading /数据/客户/秘密视频.mp4") returns the input unchanged; /data/客户/secret.mp4 also remains unchanged. redactKnownPaths protects the two audioPadTrim paths because that caller knows them, but the generic redactor is also used for CLI telemetry and producer observation messages where no known-path list is supplied. Use Unicode-aware segment classes (or a delimiter-based path rule) and add non-Latin absolute/relative regressions.

Focused local evidence: the current contract suite passes 63/63 and telemetry-redaction suite 23/23, which confirms these cases are outside the present tests. Core CI, producer unit/integration, Test, Fallow, lint, typecheck, and CodeQL are green; three regression shards were still running when reviewed.

[vanceingalls]

New head 1664fe6ad. All three R3 findings fixed, each reproduced first.

1 — shell/Python callers excluded. Confirmed. frame_strip.sh:52-57 passed the user-controlled baseline as ffprobe's last positional with no terminator; render-and-composite.sh had four more. Both fixed (bash -n and a Python compile() check on the embedded block both pass).

On the gate, I took your first option but split it by what's actually checkable:

• .py/.sh now join the sweep. A Python list argv is a bracket literal, so frame_strip.sh gets the same position check as JS — -- must be penultimate.
• Shell command lines get a separate, explicitly weaker presence check. ffprobe … "$BG" 2>/dev/null | tr -dc '0-9.' has redirections and pipes after the input, so position-checking needs a shell parser. The test says so in its own comment rather than implying an equal guarantee.
• Narrowing that check surfaced two false positives worth noting: command -v ffprobe >/dev/null and echo "ffmpeg/ffprobe not on PATH". An invocation passes flags, so the match requires ffprobe -.

2 — non-Error rejection threw. Reproduced exactly: redactKnownPaths(undefined, …) → TypeError: undefined is not an object (evaluating 'out.split'). Normalized at the boundary with reason instanceof Error ? reason.message : String(reason), and redactKnownPaths now fails soft on a non-string — it sits on an error path, so it must not be the thing that throws. Both injected seams pinned with string/undefined/null/number/object rejections (10 cases); restoring the cast fails 4.

3 — Unicode paths verbatim. Reproduced all three of your examples. Rather than enumerate \p{L}\p{N}\p{M}… — which then has to be kept correct for marks, joiners and emoji — segments are now defined by their delimiters, which is right for every script by construction.

That exposed a second instance of the same ASCII assumption: the bare-relative lookbehind was (?<![\w/\\.:@-]), so with a non-ASCII preceding character a match could start mid-token and 客户/秘密/视频.mp4 redacted to 客户[path] — still leaking a segment. It is now a token boundary, and bare-relative runs before absolute so it claims the whole token instead of letting the absolute rule take the interior.

Verified the non-paths still survive: N/A, 24/1, 48000/1001, Stream #0:0 unchanged, and URLs keep their host.

Mutation-tested all three seams — reverting each fix fails the suite.

Suites: core 1497, producer 598, engine 1370, cli 2436, lint 514, studio-server 406. producer:test:unit lane exits 0. Contract suite 67, redaction 35.

Re-requesting review.

[miguel-heygen]

Approving at 1664fe6.

All three findings from my previous review are closed at the actual boundaries:

• Unicode path redaction now defines segments by delimiters rather than ASCII word characters. The exact prior repros redact fully, including /数据/客户/秘密视频.mp4, /data/客户/secret.mp4, /Volumes/客户 项目/秘密.mov, and /tmp/用户/private.mov.
• audioPadTrim normalizes unknown rejection reasons before sanitizing, so string, undefined, null, number, and plain-object probe rejections return a failed PadTrimAudioResult instead of rejecting from the redactor. redactKnownPaths also fails soft on a non-string.
• The source sweep now includes .py and .sh. The embedded Python argv in frame_strip.sh is checked for penultimate terminator position; direct shell lines are checked separately for terminator presence. All five shipped shell/Python misses are fixed.

Verification:

• telemetry redaction plus argv contract: 102/102 focused tests pass locally.
• Mutation check: removing the frame_strip.py terminator and one render-and-composite.sh terminator produced two named contract failures; restoring them returned 67/67.
• Exact-head Producer unit CI is green, covering the public audioPadTrim rejection regressions.
• Worktree is clean and HEAD is exactly 1664fe6.

CI is still running: 44 success, 2 skipped, 12 pending, 0 failures at review time. Approval clears the code-review block; required checks still gate merge.

Review by Magi.