fix(render): consolidate preflight and local recovery

[miguel-heygen]

Summary

• fall back to an available hardware H.264 encoder when FFmpeg lacks libx264
• clean orphaned browser trees before local render retries
• check free space on the actual render write volumes
• preserve resolved FFmpeg and FFprobe binary paths in encoder/probe tests
• supersede fix(render): fall back when libx264 is unavailable #2373, fix(render): recover orphaned browsers before retry #2342, fix(render): check disk space on write volumes #2232, and fix(engine): resolve ffmpeg/ffprobe via a PATH scan when where/which fails #1882; the PATH-scan implementation from fix(engine): resolve ffmpeg/ffprobe via a PATH scan when where/which fails #1882 is already present on main and remains unchanged

Verification

• full CLI suite: 1,737 passed, 2 skipped
• focused CLI render/preflight tests: 73 passed
• focused engine encoder/binary/probe tests: 69 passed
• CLI and engine typecheck
• full engine suite: 975 passed, 3 skipped; 2 environment-only failures because the host FFmpeg does not support -fps_mode

[vanceingalls]

Consolidation review from the runtime-interop + cross-layer contract lens. Rames covering test + observability; not duplicating.

Audited: packages/cli/src/commands/render.ts (renderLocal + imports), packages/cli/src/browser/ffmpeg.ts, packages/cli/src/browser/preflight.ts, packages/cli/src/utils/orphanCleanup.ts, packages/engine/src/services/streamingEncoder.ts (200-310, 440-450), packages/engine/src/utils/gpuEncoder.ts, packages/engine/src/utils/ffmpegBinaries.ts at main (to verify the #1882 claim).

Trusting: the four source PRs' individual test coverage; Rames's peer lens.

Absorption preserves the four proposed contracts:

• #2373 libx264 fallback → resolveH264EncoderMode + detectH264EncoderMode exported from packages/cli/src/browser/ffmpeg.ts:19, wired into renderLocal at packages/cli/src/commands/render.ts:1483.
• #2342 orphan recovery → killOrphanedProcesses() unconditional at packages/cli/src/commands/render.ts:1444.
• #2232 write-volume disk check → diskPaths option on runEnvironmentChecks, renderLocal passes [tmpdir(), dirname(outputPath)].
• #1882 PATH-scan → present on main unchanged (packages/engine/src/utils/ffmpegBinaries.ts verified); the test files touched here re-anchor basename()-shape assertions to that landed resolved-path behavior. Consistent.

Ordering inside renderLocal (orphan cleanup → preflight → env-var wiring → encoder detection) is safe: they don't share mutable state, and encoder detection reads the resolved preflight.ffmpegPath only after preflight has run.

Strengths

• packages/cli/src/commands/render.ts:1444 — orphan cleanup at the top of renderLocal means every retry path picks up recovery for free, no arm-the-retry plumbing needed.
• packages/cli/src/browser/preflight.ts:211-215 — injecting freeDiskMb makes checkDisk unit-testable without hitting the filesystem, and the multi-path extension slots into the same seam cleanly.
• packages/cli/src/browser/ffmpeg.ts:19 — separating pure resolveH264EncoderMode from detectH264EncoderMode (execFile) is the right seam; the pure function is what's covered by the unit tests.

Important

1. packages/cli/src/commands/render.ts:1483 — the libx264 flip is a compile-time presence check; the engine's GPU decision is a runtime usability probe. Mismatch bites on Linux.

   The CLI's detectH264EncoderMode regex-scans ffmpeg -encoders output — passing means the encoder is compiled in, not that it works at runtime. The engine's packages/engine/src/utils/gpuEncoder.ts:62 detectGpuEncoder does the real check (spawn + canUseGpuEncoder test encode). On a Linux build compiled with --enable-vaapi but running in a container without /dev/dri/renderD128 (also NVENC-without-NVIDIA-driver, QSV-without-Intel-silicon), the CLI flips options.gpu = true and prints "falling back to the available H.264 hardware encoder", then packages/engine/src/services/streamingEncoder.ts:445 getCachedGpuEncoder() returns null, shouldUseGpu evaluates false at streamingEncoder.ts:250, and the software branch at streamingEncoder.ts:304 hardcodes libx264 — the exact encoder the CLI thought was missing. FFmpeg then errors Unknown encoder 'libx264'. Same failure outcome as pre-PR, but with a preceding log line that misleads about what happened.

   The motivating case (macOS h264_videotoolbox without libx264) is safe — VideoToolbox is a system framework and always usable on macOS. The false-positive is Linux-only.

   Fix options: (a) restrict the CLI-side flip to videotoolbox only, which matches #2373's stated regression scenario and stops stepping on the engine's job for the rest; or (b) call getCachedGpuEncoder() and flip only when it returns non-null.

2. packages/cli/src/browser/ffmpeg.ts:29-36 invoked at packages/cli/src/commands/render.ts:1484 — new preflight throw is unwrapped.

   detectH264EncoderMode calls execFileSync(ffmpegPath, ["-hide_banner", "-encoders"], { timeout: 5000 }). resolveH264EncoderMode throws when neither libx264 nor a whitelisted hardware encoder is present. Neither throw is caught at the call site, and neither goes through errorBox() or getFFmpegInstallHint(). execFileSync timeouts (AV scanner cold-scanning the binary on Windows Defender first exec, NFS-mounted ffmpeg, cold Rosetta first exec) will now abort renderLocal with a bare stack instead of letting the render proceed to the real encode call where FFmpeg would surface its own message.

   Fix: try/catch the call; on any exception default to "software" and let FFmpeg's own error surface at the real encode call — same failure path as pre-PR, no new failure mode added.

Nits

• packages/cli/src/commands/render.ts:1455 — [tmpdir(), dirname(outputPath)] drops projectDir from disk coverage. Intentional per #2232's stated thesis ("check write volumes, not the project volume"), so this is a nit not an important. But if projectDir sits on a distinct filesystem from tmpdir AND anything the render pipeline writes lands there (per-project asset extraction, .cache/, transient bundle), the pre-PR coverage of that path is now gone. diskPaths: [tmpdir(), dirname(outputPath), projectDir] gets it back — packages/cli/src/browser/preflight.ts:267 dedupes via new Set so same-volume cases collapse.
• packages/cli/src/browser/ffmpeg.ts:24 — /\blibx264\b/ scans both the encoder-name column and the description column. A build shipping libx264rgb alone would false-positive on the description string; in practice libx264 and libx264rgb ship together, so this is defensive-only. Pinning to the name column (/^\s*[VASF][.\w]*\s+libx264\b/m) would be more robust.
• packages/cli/src/utils/orphanCleanup.ts:90 — when getUid() returns null, userFlag is "" and the pgrep -f on line 93 runs unscoped. Fine on single-user machines; on containers running as root inside a shared PID namespace, sibling processes can be swept. Consider bailing (return 0) when uid === null.
• No telemetry event fires when the CLI-side flip lands. If Important #1 hits in the field, we have no signal — the misleading warn line goes to stdout and disappears. A one-liner logEvent on the flip (and on the flip-plus-engine-null downstream case, if you accept option (b) for the fix) would surface Linux false-positives.
• No unit test covers the detectH264EncoderMode throwing path (execFileSync failure or "neither encoder present"). Adds up with Important #2 — coverage of the failure path is what makes the try/catch safe to add. Rames's coverage lens probably belongs here too.

Verdict: COMMENT
Reasoning: No blockers; CI is green and all four source contracts are preserved. Two cross-layer / cross-slice concerns worth addressing before or fast-follow — the libx264 flip in particular steps on the engine's usability probe on Linux and produces a misleading log without changing the eventual failure outcome. Miguel's call on urgency.

— Via

[james-russo-rames-d-jusso]

Reviewed at cd308e5d34b7d1cac98fca3e11dfd70aee7c12dd — consolidates #2373 / #2342 / #2232 / #1882. PATH-scan from #1882 already on main, unchanged.

Four independent wins, cleanly composed

1. Hardware H.264 fallback — resolveH264EncoderMode parses ffmpeg -encoders output; when libx264 is absent but one of videotoolbox|nvenc|vaapi|qsv|amf is available, renderLocal flips to GPU mode with a console.warn diagnostic. Word-boundary regex (\blibx264\b) avoids collisions with libx264rgb. Throws when neither is present — surface message is actionable.
2. Orphaned-process cleanup — killOrphanedProcesses in packages/cli/src/utils/orphanCleanup.ts scans chrome-headless-shell, chrome_headless_shell (both hyphen/underscore variants), and puppeteer_dev_chrome_profile — kills entire subtrees only when PPID=1 (real init-reparented orphans), scoped to the current user (pgrep -u <uid>), SIGTERM then SIGKILL after 500ms grace. Windows-safe no-op. Won't touch active user browsers on shared hosts.
3. Multi-volume disk check — runEnvironmentChecks({ diskPaths: [tmpdir(), dirname(outputPath)] }) now checks BOTH the temp scratch volume AND the output volume. checkDisk refactored to accept an explicit path plus injectable freeDiskMb (nice testability). Detail string includes the actual path — resolves the "which volume is low?" ambiguity.
4. Test-side FFmpeg/FFprobe path resilience — basename(...) compares against ffmpeg / ffprobe regardless of resolved absolute path — fixes CI runs where system tools resolve to different absolute paths than the mocked ones.

Verification

• resolveH264EncoderMode test coverage — .ffmpeg.test.ts:57-66 covers the VideoToolbox fallback happy path. Missing edge: what happens when neither libx264 nor a hardware encoder is present (the throw). Not blocking — the throw path surfaces via preflight failure, and the compose test at render.test.ts:359-372 exercises the fallback branch end-to-end.
• killOrphanedProcesses scoping — verified -u <uid> gate + PPID=1 orphan check + subtree kill. puppeteer_dev_chrome_profile marker means dev-mode Chrome is only killed when it's a genuine orphan, not while an active Puppeteer parent still holds it.
• options = { ...options, gpu: true } mutation — reassigns the local binding at render.ts:1488; downstream code paths that read options.gpu see the flipped value. Consistent with the message "falling back to the available H.264 hardware encoder".
• Cross-cutting mocks in render.test.ts — new orphanCleanupState and ffmpegEncoderState hoist correctly and reset in beforeEach. No test isolation issues visible.

Adversarial pass

• isOrphan(pid) checks PPID at time-of-check, not time-of-kill. A process re-adopted between the check and killProcessTree would be false-positive killed. Realistic window is microseconds — not a real concern.
• _cachedUid never invalidated. Node processes don't change UID mid-run except via privileged setuid, which HF CLI never does. Safe cache.
• Hardware encoder ordering. \bh264_(?:videotoolbox|nvenc|vaapi|qsv|amf)\b doesn't include h264_omx (Raspberry Pi) — likely not a target platform for HF; low realism.
• detectH264EncoderMode(ffmpegPath, false) gpuRequested=false is hard-coded. The user-requested-GPU path already exits early because the outer guard is if (!options.gpu && ...). So the gpuRequested param is effectively vestigial in this call site — that's fine; keeping it in the primitive keeps future call sites flexible.
• pgrep -f ${processName} with hardcoded strings — no shell-injection risk.
• Preflight ffmpegPath optional — renderLocal guards if (... && preflight.ffmpegPath). Correct.

Verdict framing

Four concerns, well-composed, correctly scoped, safely tested. LGTM from my side.

— Review by Rames D Jusso

[miguel-heygen]

Addressed Via’s two current-head findings in c9f2083: the automatic non-libx264 fallback is now limited to VideoToolbox (the motivating macOS contract), and advisory capability-probe failures fall through to the real encode instead of aborting with a bare stack. Added regressions for Linux VAAPI false-positive avoidance and probe-failure fallback. Focused result: 64 tests pass; CLI typecheck/lint/format pass.

[vanceingalls]

Position: RIGHT — both R1 findings resolved cleanly at c707dd70, with dedicated tests.

R2 re-review at c707dd70b94c374401f6e075c202b1c33d62f97d. Adversarial pass on the encoder-routing + probe-throw surface per the follow-up-landed-in-scope discipline; no substantive new blockers surfaced.

R1 disposition

• R1 #1 (compile-time vs runtime probe mismatch) — resolved. packages/cli/src/browser/ffmpeg.ts:20-27 now whitelists h264_videotoolbox only; NVENC / VAAPI / QSV / AMF are removed from the hardware-encoder fallback trigger. Linux VAAPI-compiled-no-device no longer flips the CLI. packages/cli/src/browser/ffmpeg.test.ts:74-79 pins this: resolveH264EncoderMode given h264_vaapi output throws "neither libx264 nor VideoToolbox" rather than promoting to gpu. Warning text at render.ts:1493 was correspondingly narrowed to name VideoToolbox specifically. This is exactly R1's suggested option (a).

• R1 #2 (uncaught execFileSync / "neither encoder" throw) — resolved. packages/cli/src/commands/render.ts:1485-1490 wraps detectH264EncoderMode in try/catch, defaults encoderMode="software", with a comment explicitly deferring to the authoritative FFmpeg error at real encode. packages/cli/src/commands/render.test.ts:381-395 covers the throw path end-to-end through renderLocal (probe-timeout error → useGpu stays false). Cold-scan / NFS / Rosetta / hung-shim scenarios all funnel to the same graceful degradation.

Adversarial delta (fresh sub-agent pass)

Fresh scenarios against the R2 encoder-routing surface. Cleared: probe hang via 5s timeout, Rosetta cold-exec throw, bad-PATH-entry hang, test coverage of the try/catch end-to-end through the mocked call, no other production callers of detectH264EncoderMode, orphan cleanup ordering doesn't touch the probe path. Residuals below.

Important (nice-to-have follow-ups, none R2-added)

1. packages/cli/src/browser/ffmpeg.ts:25 — resolveH264EncoderMode short-circuits on gpuRequested=true and returns "gpu" regardless of whether the encoders string contains h264_videotoolbox at all. The current sole caller at render.ts:1486 passes false, so this branch is unreachable today. But the exported signature would silently re-introduce the R1 #1 pattern for any future caller passing true on a non-VideoToolbox build — bypassing the very restriction R2 was designed to enforce. Consider either (a) removing the gpuRequested parameter entirely since the sole call site hardcodes false, or (b) making the parameter's contract explicit ("assume caller has already runtime-verified the encoder") in a comment. Nit-adjacent; API-shape hygiene.

2. Windows/Linux users on non-libx264 builds with h264_amf / h264_nvenc / h264_qsv / h264_vaapi compiled in now hit a slightly different failure mode. Pre-R2 (as originally proposed): CLI flipped to gpu, engine's runtime probe was authoritative, worked when runtime-usable. Post-R2: resolveH264EncoderMode throws "neither libx264 nor VideoToolbox", render.ts:1487 catches, defaults to software, engine skips the GPU probe, hardcoded libx264 misses. This trades a Linux VAAPI false-positive for the loss of Windows AMD (and Linux NVENC/QSV) automatic fallback. That's the right trade for the false-positive prevalence, but the PR body doesn't call it out; users on those builds now need to pass --gpu explicitly to reach the engine's runtime probe. Worth a note in the CHANGELOG or hint text at the throw site.

Nits

• packages/cli/src/commands/render.ts:1493 — the "falling back to VideoToolbox H.264 encoding" log fires before the engine's runtime probe. In the residual edge case where VideoToolbox is compiled in but genuinely unusable at runtime (unsigned binary, VT-disabled policy container), the log will still print, then FFmpeg errors Unknown encoder 'libx264' at real encode. Vanishingly rare on mainstream macOS; noted for completeness. Same failure class as R1 #1 but scoped to a macOS edge case I already flagged as safe in R1.
• The remaining R1 nits (projectDir dropped from disk coverage, libx264rgb false-positive, pgrep -f unscoped when uid=null, missing telemetry on the flip) were R1-tagged as nits and remain untouched by R2. Not gating.

CI

35 required checks green, 8 regression-shards pending (optional cadence). No CI-gate blockers. mergeable_state=blocked reflects REVIEW_REQUIRED only.

Verdict: COMMENT
Reasoning: Both R1 findings addressed with dedicated regression tests. Adversarial pass surfaces no R2-introduced blockers — the residuals are either pre-existing scope trade-offs or API-shape nits. Miguel can approve with confidence; the ffmpeg.test.ts VAAPI test locks in the intended narrowing.

— Via

[james-russo-rames-d-jusso]

Reviewed at c707dd70b94c374401f6e075c202b1c33d62f97d — delta from my R1 (cd308e5d).

R2 delta scope

Two behaviorally-relevant commits over the R1 branch (Fallow chore aside):

1. 9aaa9334 — narrow GPU fallback + advisory probe (packages/cli/src/browser/ffmpeg.ts, packages/cli/src/commands/render.ts)

   • resolveH264EncoderMode: fallback regex tightened from h264_(?:videotoolbox|nvenc|vaapi|qsv|amf) → h264_videotoolbox only. Error message updated to match.
   • renderLocal line 1481: detectH264EncoderMode(preflight.ffmpegPath, false) now wrapped in try/catch — probe failure defaults to "software" (i.e. useGpu stays false) so the actual encode surfaces the authoritative FFmpeg error.
   • Regressions added: ffmpeg.test.ts VAAPI false-positive throws; render.test.ts "encoder probe timed out" case asserts useGpu === false.

2. c707dd70 — Fallow scoping (preflight.ts, streamingEncoder.test.ts)

   • fallow-ignore-next-line complexity on runEnvironmentChecks; fallow-ignore-file code-duplication on the streamingEncoder test file. Comment-only, no runtime impact.

Verification

• Narrowing is well-motivated. The doc-comment on resolveH264EncoderMode (lines 15-19) states the fallback exists because macOS FFmpeg from Homebrew ships VideoToolbox without libx264. VideoToolbox is macOS-only; the other four GPU encoders (nvenc, vaapi, qsv, amf) either can't run on macOS or, on Linux, get listed by ffmpeg -encoders without being functionally available (VAAPI in a container without hardware pass-through is the canonical false-positive). Narrowing to the encoder that actually matches the scenario the fallback exists for is correct.
• Advisory pattern already exists in findOnPath (ffmpeg.ts:59-71, catch { return undefined }) and readToolVersion (preflight.ts:63-75, structured {ok: false}). The R2 change brings detectH264EncoderMode into line with those siblings — resolveH264EncoderMode was the sole capability probe that threw uncaught to the render caller. Consistency improvement, not a new pattern.
• Test load-bearing. render.test.ts:382-395 sets ffmpegEncoderState.error to force detectH264EncoderMode to throw and asserts the render still proceeds with useGpu === false. Would fail against pre-fix code (unhandled throw → renderLocal rejects).
• Error-string coupling. The new test asserts "neither libx264 nor VideoToolbox" substring — matches the updated exception text. Test and prod paths edited together.

Adversarial pass — Miguel's "different conclusions" ask

• Silent-catch diagnostic loss. render.ts:1484 swallows probe errors with a bare catch {} + explanatory comment. If a user reports "GPU fallback didn't trigger on my Mac", the probe error is gone — no way to distinguish "libx264 was present so we correctly stayed software" from "probe timed out and we defaulted software." The sibling readToolVersion surfaces the reason in detail. Suggest: log the probe failure to stderr behind !quiet, matching the render command's other diagnostic-warning idioms (console.warn(c.warn(...))). Small change, meaningful for user-reported failures. Extrapolatable across the render capability-probe surface — apply here if the same pattern spreads to other probe fallbacks.
• Behavioral break for non-macOS libx264-missing users. Anyone who was relying on the old code's implicit NVENC/VAAPI/QSV/AMF fallback path (e.g. hand-compiled minimal FFmpeg on a Linux GPU box, uncommon but possible) will now get "neither libx264 nor VideoToolbox" and fail. This is the correct behavior — those users should either install libx264 or explicitly pass --gpu — but the diff should probably ship a changelog entry naming the narrowing. I don't see docs/changelog.mdx touched in either of the two R2 commits. Consider adding a one-liner ("H.264 auto-fallback now only triggers on VideoToolbox; other builds must install libx264 or use --gpu") so the release notes are honest about the shift.
• detectH264EncoderMode's 5s timeout unchanged. The probe still has timeout: 5000 inline. This was likely the probe-timeout regression case Magi's summary mentions. Not concerning — 5s for a ffmpeg -encoders invocation is generous. Just noting the timing hasn't moved.
• c.warn wrap. The updated warn message is now single-line-wrapped inside c.warn(...). Style-clean.
• Fallow annotations. fallow-ignore-next-line complexity on runEnvironmentChecks is a scope-of-technical-debt call, not something to hold up a rendering fix. Similarly the code-duplication file-scope on the streamingEncoder test. Both are honest "known and scoped for follow-up" markers.

Verdict framing

Consistency correction on the capability-probe pattern + a well-motivated fallback narrowing. Two soft notes I'd address here rather than follow-up: (a) a diagnostic stderr line where render.ts:1484 currently swallows the probe error, and (b) a one-line changelog acknowledging the Linux/GPU-fallback behavior shift. Neither blocks landing. LGTM from my side.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

Reviewed at 824ddcf4bc5e08dc74235984e6c94256b1f8bc7e — R3 delta from my R2 (c707dd70).

R3 delta scope

Both extrapolation seams from my R2 closed in one commit (824ddcf4):

1. Diagnostic stderr on advisory probe failure — render.ts:1487-1493:

   } catch (error) {
     // Capability probing is advisory. Let the real encode surface the
     // authoritative FFmpeg error instead of failing here with a bare stack.
     if (!options.quiet) {
       const detail = error instanceof Error ? error.message : String(error);
       console.warn(c.warn(`  Unable to probe H.264 encoder capabilities: ${detail}`));
     }
   }

2. Changelog entry — docs/changelog.mdx:26:

   Render: Limit automatic missing-libx264 fallback to macOS VideoToolbox; other hardware encoders now require explicit GPU configuration and surface their native FFmpeg error.

Verification

• Diagnostic emission is quiet-mode-gated. Two new tests in render.test.ts:383-414 prove both branches:
  • "lets the encoder surface its own error when capability detection fails" (quiet: true) — spies console.warn and asserts it was NOT called with "encoder probe timed out". Confirms quiet path suppresses.
  • "diagnoses advisory encoder probe failures unless quiet" (quiet: false) — same setup, asserts warn WAS called with "encoder probe timed out". Confirms non-quiet path surfaces.
    Load-bearing pair — the two tests cover the exact behavior split the fix introduces.
• Non-Error safety. error instanceof Error ? error.message : String(error) handles the theoretical "thrown non-Error" case (matches the pattern I noted on #2407's R3). No undefined in the log if something weird is thrown.
• c.warn(...) wrapper matches sibling idiom — the existing "FFmpeg does not include libx264; falling back to VideoToolbox H.264 encoding" warning uses the same console.warn(c.warn(...)) shape. Style-consistent.
• Two-space indent on the warn string matches the existing sibling warning below at render.ts:1499 (" FFmpeg does not include libx264..."). Preserves the render-log visual formatting.
• Changelog entry names the behavior shift accurately — "other hardware encoders now require explicit GPU configuration" is the exact fallout for Linux/NVENC/VAAPI/QSV/AMF users on hand-compiled minimal FFmpeg. Users hitting the new error can look up the changelog and understand why the fallback narrowed.

Adversarial pass

• options.quiet truthiness. renderLocal reads options.quiet unconditionally without a default; if a caller omits it, !options.quiet is true and the warn fires — which is the conservative choice (more diagnostics, not fewer). No regression risk.
• c.warn on error paths. The c.warn chalk styling is applied to stderr. If a downstream tool grep-parses stderr for probe errors, they'll see the ANSI codes — but chalk respects CI=true and NO_COLOR env vars, so CI logs will be plain text. Consistent with the sibling warning.
• Test spy leakage. vi.spyOn(console, "warn").mockImplementation(() => undefined) in each test — vitest auto-restores after the test (afterEach implicit) so no bleed across tests. Correct.
• Advisory-probe pattern coverage. I noted in R2 that the sibling probes (findOnPath, readToolVersion) already handle failure defensively — readToolVersion returns {ok: false, detail: ...} which the caller can log. detectH264EncoderMode's probe now matches that "capture the failure, expose the reason" contract. Pattern parity across the render capability-probe surface is now complete.
• error instanceof Error guard in the catch — matches the pattern I called for in #2407 R3 (error?.message ?? String(error)). Cross-PR consistency is now aligned.

Verdict framing

Clean R3 — both extrapolation seams from R2 addressed with load-bearing regression pairs (quiet-mode branch + non-quiet-mode branch) and an accurate changelog entry documenting the Linux/GPU-fallback behavior shift. Nothing to hold. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

Position: RIGHT — both R2 residuals landed at 824ddcf4b with load-bearing regression pairs, and the extrapolation-blocker rule doesn't fire against the sibling CLI probe surface.

R3 re-review at 824ddcf4bc5e08dc74235984e6c94256b1f8bc7e — delta from my R2 (c707dd70, pullrequestreview-4699325245). Runtime-interop + advisory-probe-surface lens; test/observability coverage already stated by Rames R3, not duplicating.

Per-residual disposition

Both R2 adversarial residuals CLOSED at head:

1. R2 residual — silent probe-throw swallow at render.ts:1485 — CLOSED. Diff at render.ts:1487-1493 now:

   } catch (error) {
     // Capability probing is advisory. Let the real encode surface the
     // authoritative FFmpeg error instead of failing here with a bare stack.
     if (!options.quiet) {
       const detail = error instanceof Error ? error.message : String(error);
       console.warn(c.warn(`  Unable to probe H.264 encoder capabilities: ${detail}`));
     }
   }

   Quiet-mode gate is correct: options.quiet is CLI-arg-sourced (no env-var overload — render.ts:631, const quiet = args.quiet ?? false), so !options.quiet gates precisely on the --quiet flag the user set. Non-Error throw is safe (instanceof Error ? .message : String(error) — null/undefined/thenable all stringify without crash). Test pair render.test.ts:383-414 asserts both branches: quiet:true → warn NOT called with "encoder probe timed out", quiet:false → warn CALLED with same substring. Load-bearing.

2. R2 residual — scope trade-off for non-libx264 non-VideoToolbox users — CLOSED. docs/changelog.mdx:26 under v0.7.58 Fixes:

   Render: Limit automatic missing-libx264 fallback to macOS VideoToolbox; other hardware encoders now require explicit GPU configuration and surface their native FFmpeg error.
   Names the shift accurately: Linux VAAPI / NVENC / QSV / AMD AMF users on hand-compiled minimal FFmpeg now hit the throw at ffmpeg.ts:26 ("neither libx264 nor VideoToolbox H.264 encoding") and the new advisory warn if capability probing itself throws. Users hitting the new failure mode have a discoverable changelog entry to reference. ffmpeg.test.ts line ~70 pins the throw against a h264_vaapi-only encoders string, so the scope-narrowing is enforced by unit test, not documentation alone.

Extrapolation-blocker check (Miguel's rule)

Class-shape: "advisory probe with silent-fallback that swallows the failure reason." I greped the CLI probe surface at head for the same shape and checked whether the fix extrapolates.

Site	Shape	Verdict
render.ts:1487 detectH264EncoderMode	Advisory probe → silent fallback to software	Fixed by this PR (reason on stderr, quiet-gated).
ffmpeg.ts:66 findOnPath — try { which/where } catch { return undefined }	Search probe → "not found" fallback surfaces via checkFFmpeg outcome + install hint	Different class. The catch swallows the failure BUT the failure semantic IS "not found," which is what the preflight outcome layer expresses with an actionable install command. Not the silent-fallback class this PR targets.
preflight.ts:208 checkChrome — try { info = await findBrowser() } catch { info = undefined }	Search probe → "Chrome not found" outcome	Different class. The catch is documented in an in-code comment as the deliberate design ("that is the exact condition this check exists to report"); failure surfaces via preflight outcome + npx hyperframes browser ensure hint. Not silent-fallback.
preflight.ts:59 readToolVersion	Reason-capturing probe returning {ok:false, detail:...}	Already reason-preserving. No gap.
preflight.ts:243 checkDisk/getFreeDiskMb	Returns null on failure, outcome shows "Unable to check"	Different class. Non-actionable reason (statfs errno rarely user-fixable); "Unable to check" is the intent-preserving surface.

Conclusion: the fix's class ("advisory probe that silently swaps behavior on failure") is uniquely exhibited by detectH264EncoderMode on the CLI surface. The sibling try/catch → return undefined shapes all land on a checked outcome-layer with an install command — deliberately-designed "not-found" fallbacks, not silent-behavior-swap fallbacks. Extrapolation-blocker rule does not fire. No REQUEST_CHANGES tier.

Adversarial delta (Rule 15 fresh-lens)

Scenarios I ran against the stderr-emit fix:

• Non-Error throw — throw null, throw undefined, throw "string", throw {}. String(error) handles all four ("null", "undefined", "string", "[object Object]"). No crash, no confusing empty-message emission. ✅
• --quiet via env var vs --quiet via CLI arg — traced: quiet is CLI-arg-sourced only (render.ts:631 args.quiet ?? false); no HYPERFRAMES_QUIET env-var overload exists. Gating on !options.quiet correctly covers the only user-controllable surface. ✅
• chalk / CI stderr — c.warn(...) respects CI=true and NO_COLOR, so the message emits as plain text on CI. Not suppressed. ✅
• Test spy bleed — vitest auto-restores vi.spyOn(console, "warn") across tests; no cross-test contamination. ✅
• Emission before encoderMode remains "software" — after the catch, encoderMode retains its "software" default (line ~1486), skipping the if (encoderMode === "gpu") branch that would have swapped options.gpu = true. Silent-swap contract preserved for the software fallback path; only the reason now surfaces. ✅

Adversarial pass cleared.

Divergence from Rames R3

Rames covered the diagnostic emission + quiet-branch tests + non-Error safety + chalk CI handling + cross-PR pattern parity with readToolVersion (positive: same reason-capture contract).

My divergence: the negative extrapolation check against findOnPath / findBrowser / getFreeDiskMb. Rames confirmed positive pattern parity with one sibling probe (readToolVersion already surfaces reason). I confirm the class boundary against the three siblings that ALSO silently swallow reasons via catch { ... return undefined } — those are a different class (not-found fallback with actionable outcome), not the silent-behavior-swap class this PR targets. Extrapolation-blocker rule doesn't apply outward from this fix.

Belt-and-suspenders overlap acknowledged on: non-Error instanceof Error ? .message : String(error) safety pattern.

CI

At head 824ddcf4b: mergeable_state=blocked, no required check failing. Passed so far: Analyze (actions/python), CLI npx shim (macos + ubuntu), Fallow audit, File size check, Format, Lint, Preflight, SDK, Semantic PR title, Studio load smoke, Validate docs, player-perf. Still pending: Build, Producer unit + integration, Test, Typecheck, Analyze (javascript-typescript), CLI: npx shim (windows-latest), CLI smoke (required), Render/Tests on windows-latest, Preview parity, Preflight repeats. No red required — the blocked state is a mix of pending required checks + reviewer gate. Would convert to approve on CI green if requested.

Verdict

Verdict: COMMENT
Reasoning: Both R2 residuals resolved cleanly at 824ddcf4b with load-bearing regression pairs (quiet-mode branch split + throw-scope changelog); adversarial pass on non-Error / env-var / CI-stderr scenarios all clear; extrapolation-blocker check across the CLI probe surface confirms the fix's class boundary (advisory silent-behavior-swap) is not exhibited elsewhere. RIGHT position, not APPROVE only because CI still has pending required checks — happy to convert to approve on CI green.

— Via