fix(cli,core,lint,producer,studio-server): terminate ffprobe options everywhere, pin the contract - #2945
fix(cli,core,lint,producer,studio-server): terminate ffprobe options everywhere, pin the contract#2945vanceingalls wants to merge 4 commits into
Conversation
…site #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) <noreply@anthropic.com>
…ontract 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) <noreply@anthropic.com>
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
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:
-
packages/producer/src/utils/ffprobeArgvContract.test.ts:20does 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 checksSCANNED.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,audioPadTrimshould validate--is penultimate rather than merely present. -
packages/producer/src/services/render/audioPadTrim.ts:466still exposes many exact input paths through the new error path.redactTelemetryStringdoes not cover relative paths or arbitrary absolute roots. Real ffprobe stderr for-customer-secret-intro.mp4is returned unchanged, as are/data/acme-secret/video.mp4andcustomer/acme-secret/video.mp4, and that string propagates viaPadTrimAudioResult.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.
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
2 participants
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.mp4as an option.Reproduced on ffprobe 8.1.1:
producer/services/render/audioPadTrim.ts×2Missing argument for option 'intro.mp4'— mid-rendercli/commands/init.tshyperframes initcli/whisper/transcribe.ts×2cli/utils/webmAlphaCheck.tscore/mediaGradeAnalyzer.tsproducer/plan-parity-analysis.tslint/hevcPreviewLint.tsproducer/utils/audioRegression.tsstudio-server/mediaValidation.ts,mediaMetadata.tsEleven sites, all terminated.
The part that matters
ffprobeArgvContract.test.tsscans every known caller for a format flag followed directly by a bare identifier.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 contract test also asserts its coverage list has not shrunk, so deleting an entry to make it pass is itself a failure.
Removing any single terminator fails it by name — verified against
audioRegression.ts.The clone that couldn't take the fix
audioPadTrim.ts'srunFfprobeJsonis a near-verbatim copy of the engine'srunFfprobeand structurally cannot add--itself, because callers bake the input path intoargs. It now asserts the terminator is present, takes the samestdio: ["ignore", ...]as the engine helper, and redacts its stderr — it was throwing raw ffprobe output, which echoes the input path, into logs and telemetry.Collapsing that duplication into the engine helper is worth doing, but not here: the two have diverged in error-message prefix and caller contract.
Verification
engine 1299, lint 511, core 1431, studio-server 398, cli (init/webmAlphaCheck/whisper) 146, producer utils 51, audioPadTrim 18. Typecheck and lint clean across all six packages.
🤖 Generated with Claude Code