fix(engine): preserve bounded ffprobe diagnostics

[jrusso1020]

What

• centralize engine ffprobe calls on -v error instead of suppressing all diagnostics with -v quiet
• retain only the final 8 KiB of stderr, replace the exact input path, apply the existing telemetry redactor, and cap the surfaced diagnostic at 4 KiB
• continue to ignore stderr when ffprobe exits successfully
• cover paths containing spaces, tail preservation, bounds, argument ordering, and successful probes that write stderr

Why

In the reviewed Hyperframes production window, 46 render failures reached producer_renderer_error with blank ffprobe stderr. That affected 46 workflows, 41 request IDs, and 40 parent requests. With -v quiet, we cannot distinguish invalid user media from a renderer/system failure, so these cases are not actionable and cannot be classified safely.

Safety

• no change to ffprobe stdout parsing, exit handling, cache behavior, fallback behavior, retry policy, or render routing
• no new metric tags or unbounded telemetry
• input paths are removed before the existing generic telemetry redaction
• only the retained stderr tail is surfaced, so the useful terminal error is preserved within fixed memory/error bounds

Validation

• ffprobe.test.ts: 20/20 passed
• engine suite: 1,154 passed, 3 integration tests skipped
• engine typecheck passed
• changed-file lint and format checks passed
• Fallow changed-file audit passed
• independent review: approved with no blocking findings

[vanceingalls]

Verdict: APPROVE

Tight, well-scoped change. -v error centralization is a genuine improvement over the previous per-call ["-v", "quiet", ..., filePath] shape — one place decides verbosity, one place appends the input path, and every call site drops a coupled pair of lines. Diagnostic capture is bounded twice (8 KiB stderr tail via stderrMaxBytes, then 4 KiB char cap), path is redacted before generic redaction runs, and the "successful probe with stderr" case is exercised.

Empirical premises verified against main @ base 3b3d4f5:

• redactTelemetryString — packages/core/src/telemetryRedaction.ts:21, re-exported from packages/core/src/index.ts:233. ✓
• stderrMaxBytes — packages/engine/src/utils/managedChildProcess.ts:24, 86. ✓
• No other runFfprobe callers in the tree — the signature refactor is self-contained. ✓

End-to-end validation

• User-facing path: any render that probes media (extractMediaMetadata, extractAudioMetadata, analyzeKeyframeIntervalsUncached) → runFfprobe(filePath, argsWithoutInput) → sanitizeFfprobeDiagnostic on non-zero exit.
• Coverage on the new path: packages/engine/src/utils/ffprobe.test.ts — "surfaces bounded ffprobe stderr for invalid media" (added) asserts diagnostic surfaced, input path replaced with [input], no leaked basename, size <4500, -v error present, input path is last arg. The pre-existing missing-binary fallback test at line 191 also gains an -v error check.
• CI: all Windows + Linux jobs pass on head 14a7c0c (Render on windows-latest 8m39s, Tests on windows-latest 11m18s, engine tests, Fallow audit).
• The producer half of the diagnostic-propagation story is #2769 — that PR's probeFailure/ffmpegFailure classification regexes will read this improved diagnostic. So the two PRs are complementary; merging 2772 first delivers actionable stderr immediately even for callers that don't yet consume the typed taxonomy.

P3 nits (non-blocking)

• packages/engine/src/utils/ffprobe.ts sanitizeFfprobeDiagnostic, L67-72 — stderr.split(filePath).join("[input]") only replaces exact-string occurrences of filePath. If the ManagedChildProcess 8 KiB stderr cap slices through the input path (e.g. leading noise pushes the head of the path off the retained tail), the trailing fragment survives verbatim into the redacted output. Very unlikely in practice (paths are typically <200 chars vs 8192 tail), but if you want to be belt-and-suspenders you could also run the trailing tail through a generic path regex (or basename-strip) as a second pass. Ledger-only unless we see partial paths in production diagnostics.
• packages/engine/src/utils/ffprobe.test.ts L191-214 — the "recoverable diagnostic on a successful probe" test asserts -v error and durationSeconds, which implicitly confirms stderr was ignored on success (parseProbeJson didn't throw). Consider one explicit assertion that the returned metadata doesn't leak the stderr content (e.g. expect(String(meta)).not.toContain("recoverable diagnostic")), or that no error was thrown, so the "continue to ignore stderr when ffprobe exits successfully" invariant is directly test-locked.

— Via

[miguel-heygen]

Independent exact-head review on 14a7c0cc3c349f3c5fe0b5c5733a3bedfe84e347: no additional blocking findings beyond Via’s documented non-blocking notes.

Fresh validation:

• 20/20 ffprobe Vitest cases passed.
• Real ffprobe process succeeded on a 3s PCM WAV (48 kHz, mono).
• Real invalid-media probe preserved the actionable diagnostic while removing the full input path and basename and staying within the configured bound.
• All required GitHub checks are green on this head, including Windows render/tests.

Approved.

[jrusso1020]

• fix(engine): preserve bounded ffprobe diagnostics #2772 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[jrusso1020]

Addressed both review follow-ups in d1e473a:

• explicitly asserts stderr from a successful ffprobe process is absent from returned metadata;
• redacts the basename and a leading path suffix when the 8 KiB stderr-tail boundary lands inside the input path, with a byte-accurate regression test that preserves the actionable diagnostic.

Validation: focused ffprobe tests 21/21; complete engine suite 1,155 passed / 3 skipped; engine typecheck, formatting, lint, diff check, and changed-file Fallow audit passed.

[miguel-heygen]

R2 exact-head re-review on .\n\nThe rebase is clean: shows the previously approved implementation commit is patch-equivalent ( → ), with one additive review-hardening commit.\n\nVerified both follow-ups:\n- successful ffprobe stderr is explicitly asserted absent from returned metadata;\n- exact path, basename, and an 8 KiB-tail fragment that begins inside the basename are redacted;\n- the suffix fallback is boundary-checked and only adds safe over-redaction risk, not leakage.\n\nFresh exact-head validation: 21/21 ffprobe Vitest cases pass. All CI lanes are green, including all 8 regression shards, Windows render/tests, CodeQL, CLI smoke, and global install.\n\nNo new blocking findings. Approved.

[vanceingalls]

R2 verification

Head d1e473a vs R1 14a7c0c: PR advanced with the two R1 P3 fixes plus base-branch drift. Both nits landed. Verdict: APPROVE.

Prior-finding status

Finding	Status	Evidence at head d1e473a
P3 (a) belt-and-suspenders for filePath sliced by 8 KiB stderr tail	Addressed	packages/engine/src/utils/ffprobe.ts:13-35 — new redactFfprobeInput splits stderr on the full path, then on basename(filePath), then runs a suffix-search fallback: iterate offset ∈ [1, filePath.length), take filePath.slice(offset) as a candidate suffix (skipping suffixes <4 chars), require redacted.startsWith(suffix), require the char at suffix.length to be undefined or [\s:'")\]}] (boundary), then prepend [input]. Covers the exact tail-cut case I raised.
P3 (b) strengthen "stderr ignored on success" test	Addressed	packages/engine/src/utils/ffprobe.test.ts:186-207 — successfulStderr = "recoverable diagnostic on a successful probe" is fed via stderr on a code-0 exit; test asserts expect(JSON.stringify(meta)).not.toContain(successfulStderr). Directly test-locks the "successful probe does not leak stderr into the returned metadata" invariant, matching the exact assertion I sketched. ["-v", "error"] head-args pin also present.

Fresh light-touch adversarial pass on redactFfprobeInput delta

• Boundary allowlist [\s:'")\]}]: excludes . , - ( [ {, so a suffix followed by e.g. .log won't be redacted via the fallback. That's an under-redaction against exotic wraparounds, not a leak, and the exact-path + basename passes already catch the common cases.
• Suffix minimum length 4: prevents matching trivial 2-3 char suffixes that could collide with unrelated stderr prose (e.g. .wav, .aac). Good tradeoff.
• if (!redacted.startsWith("[input]")) gate: correct — only runs the suffix fallback when neither exact-path nor basename pass already touched the head of redacted. No double-redaction.
• Complexity: O(n²) in path length (startsWith inside the loop), but paths are ≤ a few hundred chars — bounded and negligible.
• Empty-filePath guard already present at L14 (if (!filePath) return stderr;). Single-char filePath → loop body never enters (safe).
• No new runFfprobe callers introduced: rg 'runFfprobe\(' remains contained to the four in-file helpers.

Verdict

APPROVE — both P3 nits landed with exact fidelity to the suggestions; the new redaction fallback is defensible, bounded, and doesn't introduce a false-positive leak surface.

— Via