fix(cli): snapshot renders remote http(s) <video> frames (not just local files)

[xuanruli]

Problem

hyperframes snapshot works around Chrome-headless's inability to seek <video> elements by extracting a frame via FFmpeg and injecting it as an overlay (packages/cli/src/commands/snapshot.ts, the video[data-start] loop).

That path resolved <video src> only to a project-local file (resolve(projectDir, url.pathname) + existsSync) and continued past everything else. So a composition whose embedded <video> points at a remote http(s) URL (e.g. an S3-hosted clip embedded by an upstream agent) renders as a blank box in every snapshot frame — even though render shows it fine (render plays the element in-browser; the network fetch is the browser's job).

This silently breaks any downstream consumer that relies on snapshots to see the composition — e.g. an AI overlap/occlusion check reading the frames will think the video region is empty and pass an overlay that actually covers the subject.

Fix

When the src doesn't resolve to a project-local file but is an http(s) URL, pass the absolute URL straight to FFmpeg — it reads http(s) input directly. file:///blob:/data: still skip. Local-first is preserved (a project-local file always wins before the remote branch).

VP9-alpha detection (shouldUseVp9AlphaDecoder → extractMediaMetadata) shells out to ffprobe, which has no timeout, so it is only run for local inputs; for remote URLs it is skipped (pass false). This keeps the unbounded network call out of the new path — extractVideoFrameToBuffer itself remains bounded by FFMPEG_EXTRACT_TIMEOUT_MS (30s). Local alpha behavior is unchanged. (Remote VP9-alpha overlays aren't a current path; revisit with a bounded ffprobe if one appears.)

Validation

Reproduced on a real composition (hyperframes@0.7.13):

• snapshot with the embedded <video> at its remote S3 URL → blank box.
• Same composition with the clip downloaded locally + src rewritten to a relative path → video renders.
• ffmpeg -ss N -i <https-url> -frames:v 1 extracts the remote frame in ~0.5s — confirming FFmpeg handles http(s) input, which is exactly what the fix routes to.

Note: full monorepo build (tsup + runtime) not run locally; change is self-contained and type-preserving. CI build/typecheck covers it.

[miga-heygen]

Review: fix(cli): snapshot renders remote http(s) <video> frames (not just local files)

Summary: Clean, minimal fix extending snapshot's video-frame extraction to support remote http(s) URLs by passing them directly to FFmpeg, which natively handles http(s) input. Local-first invariant preserved.

Findings:

#	Location	Severity	Note
1	snapshot.ts:~375	suggestion	shouldUseVp9AlphaDecoder(ffmpegInput) runs ffprobe on remote URLs — two network round-trips per remote video per frame (ffprobe + ffmpeg). Common case (non-VP9-alpha) completes quickly and returns false. Worth noting for future optimization if snapshot perf on remote-heavy compositions becomes an issue.
2	General	nit	No test coverage for the remote path — pre-existing gap (no snapshot test file at all), not introduced by this PR. Manual validation confirmed per PR body.

Confirmed clean:

• else if placement is correct — local files always take precedence, "local-first" guarantee preserved
• file:// protocol correctly falls through to continue (not passed to FFmpeg — would be a security concern)
• SSRF surface equivalent to existing render path (Chrome fetches the same URLs in-browser) — compositions are author-controlled, no new attack vector
• Rename filePath → ffmpegInput is appropriate since the value is no longer necessarily a file path
• Type-preserving change (string | null throughout), CI passes

Verdict: LGTM — Minimal, correct, well-scoped.

— Miga

[miguel-heygen]

blocker: packages/cli/src/commands/snapshot.ts:381 now calls await shouldUseVp9AlphaDecoder(ffmpegInput) for the new remote http(s) fallback before it starts the bounded frame extraction. For remote inputs, that helper calls extractMediaMetadata(ffmpegInput), which ends up in packages/engine/src/utils/ffprobe.ts:8-11 / :265-272 spawning ffprobe with the URL and no timer/abort path. So the PR body's "existing 30s extract timeout bounds remote fetches" is not actually true: a stalled remote video can wedge hyperframes snapshot in ffprobe before extractVideoFrameToBuffer() and its FFMPEG_EXTRACT_TIMEOUT_MS timer ever run.

The remote fallback itself is the right shape and local-first resolution is preserved. The fix can stay small: either skip the VP9-alpha probe for remote URLs and pass false there (local alpha behavior remains unchanged), or add a bounded ffprobe path for extractMediaMetadata and use it from snapshot. Once that pre-extract probe is bounded or avoided for remote URLs, this is stampable.

Notes I verified: file:// still falls through instead of being handed to FFmpeg, local project files still win before the remote branch, and all current CI checks are green. Miga already noted the extra ffprobe round trip; the issue I’m blocking on is the missing timeout around that round trip.

Verdict: REQUEST CHANGES
Reasoning: The new remote path fixes the blank-frame class, but it introduces an unbounded network ffprobe before the existing 30s FFmpeg extraction timeout, so a bad remote media URL can hang snapshot instead of failing closed.

— Magi

[jrusso1020]

APPROVE — reviewed at 1f377f73. Clean, well-scoped fix.

The local-file path is unchanged — same resolve(projectDir, decodedPath) + path-traversal guard (!rel.startsWith("..") && !isAbsolute(rel)) + existsSync — so local snapshots stay byte-identical. The new else if (url.protocol === "http:" || "https:") ffmpegInput = url.href branch is the right approach: FFmpeg reads http(s) input natively, and it's parity with render (which already fetches these via the browser), so remote-embedded <video> clips (e.g. an S3-hosted clip from an upstream agent) now render in snapshots instead of a blank box. blob: / data: still correctly skip via the catch.

Checked the obvious risk for a remote input — a hang: extractVideoFrameToBuffer spawns FFmpeg with a timedOut guard, so a slow/unreachable remote URL is bounded by that spawn timeout rather than hanging snapshot indefinitely. Good.

Minor / non-blocking:

• A network-level -rw_timeout on the FFmpeg args would fail a dead remote faster + cleaner than waiting out the wall-clock spawn timeout (nice-to-have, not required).
• shouldUseVp9AlphaDecoder(ffmpegInput) now runs against the URL too, so a remote asset incurs a second network fetch (probe + extract) — negligible.
• No SSRF concern worth gating: this is a local CLI operating on the user's own composition, and render already performs the identical fetch — no new privilege boundary.

CI green.

— Rames Jusso

[xuanruli]

Good catch, @magi — you're right, the ffprobe pre-step (VP9-alpha detection via extractMediaMetadata) had no timeout, so a stalled remote host could wedge snapshot before extractVideoFrameToBuffer's 30s timer ever ran. My PR-body claim about the 30s timeout covering remote fetches was wrong for that step.

Fixed in 1a36b2a with the smaller of your two options: only probe local files for VP9-alpha; for remote URLs skip it and pass false. That removes the unbounded network ffprobe from the new path entirely — local alpha behavior is unchanged. Remote VP9-alpha overlays aren't a current path; if one shows up we can do the bounded-ffprobe option then.

(Updated the PR description to drop the inaccurate timeout claim.)

[jrusso1020]

Retracting my prior APPROVE — I was wrong, and Magi's CHANGES_REQUESTED is correct. Two mistakes on my part: I approved without first pulling the existing reviews (Magi's blocker was already standing), and my "the hang is bounded" reasoning was faulty — I checked extractVideoFrameToBuffer's timedOut guard, but that's not the path that runs first.

Verified Magi's blocker at source: for a remote http(s) input, shouldUseVp9AlphaDecoder(ffmpegInput) runs at snapshot.ts:381 before the bounded extraction, and it calls extractMediaMetadata → ffprobe.ts, which does spawn(command, args) (ffprobe.ts:11) with no timeout / abort / signal anywhere in the file. So a stalled or unreachable remote URL hangs in the unbounded ffprobe step indefinitely — the extract-stage timeout never gets a chance to bound it. My earlier note even flagged the "second probe fetch" and wrongly dismissed it as negligible; that probe IS the unbounded vector.

So this is a genuine blocker. Fix direction: bound the ffprobe spawn (a timeout/abort, ideally an FFmpeg -rw_timeout for network inputs), or skip the vp9-alpha probe for remote inputs (default false) so the only remote spawn is the already-bounded extract. Concur with Magi.

— Rames Jusso

[jrusso1020]

Re-APPROVE at 1a36b2a — the blocker is resolved; this supersedes my churn above. The new commit ("don't run unbounded ffprobe on remote snapshot inputs") fixes exactly Magi's finding: inputIsLocal now gates the probe — useVp9AlphaDecoder = inputIsLocal ? await shouldUseVp9AlphaDecoder(ffmpegInput) : false — so local files still get the (filesystem-bounded) probe, and remote http(s) inputs skip it entirely, leaving only the bounded extractVideoFrameToBuffer (its timedOut guard) as the sole remote spawn. A stalled remote host can no longer wedge snapshot in an unbounded ffprobe. Verified at source. The remote-video snapshot fix itself (http(s) fallback, local path unchanged with its traversal guard) is correct.

Apologies for the review churn — I approved prematurely (didn't pull Magi's standing review first), then mis-timed my retraction against a head that already carried the fix. Net: LGTM at 1a36b2a. CI green so far (shards still running).

— Rames Jusso

[miguel-heygen]

R2 verified at 1a36b2ab: my blocker is closed.

Strengths: packages/cli/src/commands/snapshot.ts:358-374 keeps the local-first resolution intact and only falls back to absolute http(s) inputs; file:, blob:, and data: still do not enter FFmpeg. packages/cli/src/commands/snapshot.ts:380-388 now gates VP9-alpha probing on inputIsLocal, so remote URLs no longer hit the unbounded ffprobe/extractMediaMetadata path before frame extraction.

No remaining code blockers from my lane. Remote inputs now go straight to extractVideoFrameToBuffer(...), which keeps the existing bounded FFmpeg extraction behavior, while local VP9-alpha handling remains unchanged. The remaining risk is the pre-existing lack of snapshot-specific unit coverage for this path.

CI note: core checks including Build, Typecheck, Test, Lint, and CLI smoke (required) are green at this SHA; long regression/windows/CodeQL/global-install checks are still pending, with no current failing required check observed.

Verdict: APPROVE
Reasoning: The R2 inputIsLocal gate removes the unbounded remote ffprobe call that caused my request-changes review, while preserving the intended local and remote snapshot behavior.

— Magi