fix(engine): harden ffmpeg binary resolution

[miguel-heygen]

Summary

• prefer real ffmpeg.exe / ffprobe.exe binaries over .cmd/.bat shims when PATH lookup returns multiple Windows candidates
• fall back to scanning PATH when where/which fails, instead of returning the bare binary name immediately
• add an explicit mangled-path hint when configured FFmpeg paths contain the Unicode replacement character
• make mocked missing-binary tests hide PATH explicitly now that the resolver has a real fallback scan

Repro evidence

• Added red-first coverage for the three root behaviors: Windows .cmd shim precedence, failed lookup command with valid PATH binary, and mangled replacement-character config paths.
• Before the implementation change, packages/engine/src/utils/ffmpegBinaries.test.ts failed on all three new cases.

Verification

• bun run test src/utils/ffmpegBinaries.test.ts src/utils/ffprobe.test.ts src/services/streamingEncoder.test.ts
• bunx oxfmt --check packages/engine/src/utils/ffmpegBinaries.ts packages/engine/src/utils/ffmpegBinaries.test.ts packages/engine/src/utils/ffprobe.test.ts packages/engine/src/services/streamingEncoder.test.ts
• bunx oxlint packages/engine/src/utils/ffmpegBinaries.ts packages/engine/src/utils/ffmpegBinaries.test.ts packages/engine/src/utils/ffprobe.test.ts packages/engine/src/services/streamingEncoder.test.ts
• bun run typecheck and bun run build in packages/engine
• Full bun run test in packages/engine after building upstream workspace deps: 878/880 tests passed; the remaining two failures are existing VFR extraction tests blocked by this devbox FFmpeg 4.2.7 not recognizing -fps_mode.

Related

• Consolidates the root behavior behind the Windows FFmpeg/path low-rating feedback cluster and supersedes the overlapping open/draft attempts in fix(engine): resolve ffmpeg/ffprobe via a PATH scan when where/which fails #1882, fix(engine): call out a mangled replacement character in ffmpeg path errors #1878, and fix(engine): prefer ffmpeg.exe over a .cmd/.bat shim on Windows PATH [P1] #1937.

[vanceingalls]

🟠 R1 — Windows CI is red on the new test this PR introduces; fix before merge

The chooseBestPathCandidate + scanPath machinery is a clean design for the reported Windows shim problem: where ffmpeg on some Windows installs surfaces ffmpeg.cmd (winget shim) before ffmpeg.exe, and the old code picked the first line. New behavior: prefer .exe, then extensionless, then non-cmd/bat, then whatever's first. All three fallbacks look right on the runtime-interop axis. But the new PATH-scan fallback test is currently failing on Tests on windows-latest — the failure is caused by the PR's own test authoring, and it means the CI-red gate is genuinely load-bearing here (not a flake).

Rames-bot is here under the test-coverage / observability lens.

Finding-by-finding

1. ffmpegBinaries.test.ts:64-83 fails on windows-latest — 🟠

packages/engine/src/utils/ffmpegBinaries.test.ts:78:39 at head 4f7aa0da.

From the CI log (run 28750010257):

AssertionError: expected 'D:\a\_temp\ffmpeg\ffmpeg-N-124278-gcc…' to be 'C:\Users\RUNNER~1\AppData\Local\Temp\…'
Expected: "C:\Users\RUNNER~1\AppData\Local\Temp\hyperframes-ffmpeg-path-htLxKO\ffmpeg"
Received: "D:\a\_temp\ffmpeg\ffmpeg-N-124278-gcc3ca17127-win64-gpl\bin\ffmpeg.exe"

Trace:

• Test writes a fixture at binDir/ffmpeg (extensionless, line 69) and prepends binDir to PATH (line 71), keeping originalPath after ${delimiter}.
• CI's originalPath on windows-latest contains D:\a\_temp\ffmpeg\ffmpeg-N-124278-gcc3ca17127-win64-gpl\bin — that's the ffmpeg the CI job installed (see the FFMPEG_BIN env printed at the top of the job).
• execFileSync is mocked to throw → scanPath() runs.
• On Windows, scanPath's extension iteration order is [".exe", ...PATHEXT, ""]. In binDir it hits the "" extension (finds binDir/ffmpeg); in D:\...\bin it hits .exe (finds D:\...\ffmpeg.exe).
• chooseBestPathCandidate prefers the exact ffmpeg.exe variant → picks the CI's real ffmpeg over the test's temp file.

The scanner is behaving correctly — on real Windows, an extensionless ffmpeg sitting next to a proper ffmpeg.exe should indeed prefer the .exe. The test just doesn't authorize this preference: it puts an extensionless fixture in a scanner that prefers .exe, on a CI where a real .exe exists on PATH.

Fix shape (either works):

1. On Windows, write ${ffmpegPath}.exe instead of ${ffmpegPath}:

   const ffmpegFilename = process.platform === "win32" ? "ffmpeg.exe" : "ffmpeg";
   const ffmpegPath = join(binDir, ffmpegFilename);
   writeFileSync(ffmpegPath, "");           // content doesn't matter — win32 branch uses existsSync only

2. Set PATH = binDir (drop the ${originalPath} tail) for this test only. Cleaner-scoped since the afterEach restores originalPath. On Windows some system-level lookups (system32) may still be reachable via other envs, but for execFileSync throwing you're fine — the scanner reads only process.env.PATH.

Either fix restores Windows CI green and doesn't change the shipping-code semantics under test.

2. .exe-preference is a runtime behavior change on Linux/macOS too — 🟡 (probably fine, worth double-checking)

packages/engine/src/utils/ffmpegBinaries.ts:11-25 at head 4f7aa0da.

Old findOnPath: output.split(/\r?\n/).map(trim).find(Boolean) — pick the first non-empty line of which/where output.

New findOnPath: chooseBestPathCandidate(name, output.split(/\r?\n/)) — prefer .exe variant; else exact name; else non-cmd/bat; else first.

On Linux/macOS, .exe doesn't apply; extensionless-name is preferred and matches what which ffmpeg returns 99% of the time. Regression risk exists only if:

• Someone has a symlink chain where which prints a shim path first (e.g. snap-managed ffmpeg printing /snap/bin/ffmpeg before /snap/ffmpeg/current/bin/ffmpeg). Both are extensionless → chooseBestPathCandidate picks the first. Parity with old behavior. ✅
• A user runs an obscure which that emits alternative candidates below the first. Old code took first; new code still takes first (via chooseBestPathCandidate's final fallback). ✅

The behavior on Linux/macOS looks preserved. Nit — it would be clearer to short-circuit the .exe preference on non-Windows so a reader isn't left wondering whether Linux ffmpeg named ffmpeg.exe.old-backup in a bin dir would hijack. Not a functional issue; readability nit.

3. pathEncodingHint covers only configured paths — 🟢 non-blocker

ffmpegBinaries.ts:106-118 — the mangled-Unicode hint is only surfaced when HYPERFRAMES_FFMPEG_PATH or HYPERFRAMES_FFPROBE_PATH is set and the file is missing. If PATH itself contains a U+FFFD-mangled directory, scanPath will silently miss it (existsSync returns false). Non-regression — we never had that diagnostic before — but noting for a possible follow-up: also probe pathValue.includes("�") and surface a matching warning at getFfmpegBinary fall-through.

4. Cache semantics preserved — ✅

pathCache.set(name, resolved) runs on both success and failure paths (via the shared resolved local). Matches the old dual-set pattern. No cache-invalidation regression. Cache is process-wide and never expires, which is intentional for a resolver called on every render.

5. chooseBestPathCandidate is exact-name matching — ✅

candidateFileName(candidate) === \${name}.exe`is a strict equality check, not a suffix match.ffmpeg-6.exeorffmpeg64.exewon't hijackffmpeg`. This is intentional and correct.

---

Verdict: the shipping code is correct. The PR is blocked on the failing Tests on windows-latest check for its own new test. Push a fix for the fixture (option 1 above is cleanest) and this ships clean.

Review by Via (runtime-interop lens)

[vanceingalls]

R2 — ✅ fixture fix at 556e5655 addresses the Windows-CI-red finding

Verified at head 556e5655e9938008b4bdf5eb350bfd1a69a81e2d:

packages/engine/src/utils/ffmpegBinaries.test.ts:63-84 — the "falls back to scanning PATH when which/where fails" test now:

1. Writes the fixture at ffmpeg.exe on Windows — join(binDir, process.platform === "win32" ? "ffmpeg.exe" : "ffmpeg"). Matches the scanner's .exe-first preference on Windows without changing the shipping semantics under test.
2. Isolates PATH to binDir only — process.env.PATH = binDir (previous ${binDir}${delimiter}${originalPath ?? ""} shape dropped). The CI runner's real ffmpeg on the original PATH can no longer hijack the scanner's lookup.
3. Bonus lock-in on test intent — execFileSync is now a vi.fn(() => { throw ... }), and the test asserts expect(execFileSync).toHaveBeenCalledOnce(). This tightens the contract: not only does the fallback resolve to the temp file, the mocked where/which was actually invoked and threw before falling through to scanPath. Prevents a future refactor that skips the initial execFileSync attempt from silently passing this test.

Both my R1 options landed in one commit — either would have worked; the belt-and-suspenders shape is stronger. The afterEach PATH restoration is unchanged and correctly restores original PATH from the closure-captured originalPath. Fresh vi.fn per test avoids call-count leakage between the two getMockedFfmpegBinary tests.

Windows CI at 556e565: currently pending; I'll re-verify once Tests on windows-latest completes. Given the fix is a self-contained fixture change and the shipping code is unchanged, I don't expect surprises — but noting so the merge cadence stays honest.

Runtime-interop verdict: ✅ resolved. Ready to route for stamps once CI turns green.

R2 by Via (runtime-interop lens)

[james-russo-rames-d-jusso]

Reviewed at 556e565.

🟢 R1 — LGTM, Windows FFmpeg resolution is now correctness-hardened

Traced the PostHog Windows-user feedback (ffmpeg.cmd shims winning over the real .exe; opaque failures when configured paths were mangled): fix path in packages/engine/src/utils/ffmpegBinaries.ts maps to all three root behaviors.

chooseBestPathCandidate (ffmpegBinaries.ts:11-24) — priority ladder over where/which output:

1. exact <name>.exe filename;
2. exact <name> (bare) filename;
3. anything that doesn't end in .cmd/.bat;
4. first candidate.

The .exe preference is what actually fixes the reported bug: where ffmpeg on Windows lists whatever's earliest in PATH, which for users with npm-shim'd installs is often ffmpeg.cmd. Old code did .find(Boolean) on the raw output — first line wins. New code second-guesses to prefer the real binary. That's a behavior change, but it's the intended one.

scanPath fallback (ffmpegBinaries.ts:26-50) — when where/which throws (missing PATH tools, nonzero exit on no match), walk process.env.PATH manually with extension probes. On Windows: [".exe", ...PATHEXT, ""] (from .COM;.EXE;.BAT;.CMD default); on Unix: [""] with X_OK check. Cached via pathCache so it's a one-shot cost per binary per process. The old failure mode was "return undefined immediately when where fails" — that was silently biasing users to think the binary was missing when the resolver was just not looking hard enough. Good fix.

Mangled-path hint (ffmpegBinaries.ts:98-119) — pathEncodingHint fires only when the configured env-var path contains �, appending a short message pointing at copy-paste / decode mangling. Localized, cheap, and hits the reported UX gap where users configured HYPERFRAMES_FFMPEG_PATH from a mis-decoded copy-paste and got a bare "binary not found." Good touch.

Vance already caught + resolved the Windows-CI-red on new tests (R1 → R2 at 556e5655e9, #pullrequestreview-4631770747). Re-verified: Render on windows-latest + Tests on windows-latest + all 8 regression-shards are green at HEAD.

Small notes, non-blocking:

• Extension array on Windows has .exe duplicated (leading ".exe" + PATHEXT's .EXE lowercased). Not a functional bug — chooseBestPathCandidate still returns the right one, and the file gets pushed twice into candidates but the .find() picks the first. Minor cleanup only. (nit)
• No explicit test for UNC path (\\server\share\ffmpeg.exe). candidateFileName uses .split(/[\\/]/).at(-1) which handles it correctly, but the case isn't pinned. (nit)
• scanPath walks all PATH dirs, but result caches per-binary → not a hot-path concern.

Existing-state: users stuck on .cmd-shim resolution just need to re-invoke; there's no persistent bad state to migrate. Prevention + cure both hit.

Layering on top of Vance's R1 🟠 → R2 ✅ — same read on the fixture fix. Not stamping — Magi-authored, per team protocol I defer to <@U08E7PV788Z> for the merge decision.

— Review by Rames D Jusso