fix(producer): reject asset media type mismatches

[jrusso1020]

Summary

• Classify localized assets from ffprobe streams and container signatures, then fail deterministic image/video/audio mismatches early with stable ASSET_MEDIA_TYPE_MISMATCH metadata.
• Preserve missing, corrupt, remote-at-runtime, and unprobeable assets on their existing downstream error paths.
• Run preflight after static compilation and again after browser reconciliation.
• Make probe reuse file-version-aware and render-scoped; bound the no-signal fallback cache to 128 LRU entries and limit concurrent media probes to four.
• Thread cancellation through the post-browser probe, ensure abort always wins over PNG fallback, and release the capture session plus file server on mismatch or a final cancellation race.
• Require a complete PNG envelope before accepting the missing-ffprobe metadata fallback without reintroducing synchronous whole-file scans.

Ownership and retry policy

Deterministic media-type mismatches are owner=user and retryable=false. Error transport is allowlisted and bounded; raw source URLs and paths are never included.

Validation

• Engine ffprobe/media-profile suite: 115 passing.
• Producer asset-media preflight suite: 11 passing.
• Producer probe-stage suite: 33 passing, including resource cleanup on mismatch and post-preflight cancellation.
• Engine and producer typechecks passing.
• Pre-commit tracked-artifact, lint, format, fallow, and typecheck gates passing.
• Independent adversarial re-review: no remaining blockers.

Scope

HDR behavior and dependency/version changes are intentionally out of scope.

Rollback

Revert this PR. No migration or persisted state is introduced.

[vanceingalls]

Verdict: APPROVE

The classification, cache scoping, abort ordering, and resource-cleanup contracts all check out. The regression tests you called out are semantic (not presence-only), and the error transport is genuinely bounded.

What I verified

Cache scoping — version-aware, render-scoped, real LRU (packages/engine/src/utils/ffprobe.ts:144-360)

• Identity key = dev:ino:size:mtimeNs:ctimeNs via statSync{bigint:true} (line 314-321). Truly version-identifying, not path-only.
• Render scoping via WeakMap<AbortSignal, Map<...>> (line 157) — each render's signal owns its map; cross-render leakage impossible.
• 128-entry fallback: cache-hit path does delete + set to move to newest (line 336-339); on-miss eviction deletes keys().next().value (oldest). This is a real LRU on Map insertion order, not FIFO-in-name-only. Test at ffprobe.test.ts:253-278 asserts the 128 bound behaviorally.
• Concurrent-caller dedup: probeMediaOutput synchronously stores the pending promise before await (line 342-348), so N concurrent callers on the same path deduplicate to one ffprobe. Test at ffprobe.test.ts:222-251 confirms scope isolation.
• Preflight also dedups at the byPath map (assetMediaType.ts:132) — smallest correct boundary.

Abort/cancellation — sed-swap resistant

• The "abort always wins over PNG fallback" invariant lands in two places: outer probeMediaProfile.catch re-checks options.signal.aborted before falling through (ffprobe.ts:436), AND hasCompletePngStructure calls signal.throwIfAborted() between openFile and every chunk read (ffprobe.ts:368, 384). Test at ffprobe.test.ts:356-383 ("does not turn an aborted PNG probe into a successful metadata fallback") asserts the end-to-end reject with render cancelled — real semantic assertion.
• Post-preflight cancellation: probeStage.ts:640-670 wraps preflightCompositionAssetMediaTypes + assertNotAborted() in a single try; the catch releases probeSession and fileServer before rethrowing. Regression test at probeStage.test.ts:363-376 sets afterMediaPreflight to abort mid-flight and asserts closeCaptureSessionCallCount === 1 && fileServerCloseCallCount === 1 — semantic.
• Mismatch-during-preflight release: probeStage.test.ts:347-361 — same asserts, plus mediaPreflightSignal === controller.signal proves the signal is actually threaded through.

Resource ownership on every boundary variant

• Deterministic mismatch, post-preflight cancel, "unprobeable"-returning-early, and probe subprocess timeout all flow through preflightCompositionAssetMediaTypes → probeStage catch. closeCaptureSession and closeFileServerSafely each have their own sub-catch (probeStage.ts:656-663, 665-668) so a close failure doesn't mask the original error.
• The 30s ffprobe deadline (ffprobe.ts:104) closes the "subprocess timeout" variant.

Error transport allowlist (server.ts:122-158, assetMediaType.ts:25-40)

• SAFE_RENDER_ERROR_CODES is a fixed set (4 entries). extractSafeRenderErrorMetadata returns exactly {errorCode, errorOwner, retryable} — nothing free-form. AssetMediaTypeMismatchError.message string carries only counts + sorted expected-kinds; elementFingerprint is 16-hex of sha256(id) so paths/URLs/IDs never leak. assetMediaType.test.ts:172-177 asserts JSON.stringify(err) excludes both fixtureDir and every element id.
• Grep'd the mismatch construction — every emitted field is either allowlisted or hashed.

PNG envelope — bounded, not whole-file (ffprobe.ts:362-403)

• Checks: 8-byte signature → walk chunk headers only (12 bytes read per chunk, chunkEnd = offset + 12 + declaredLen skip-past semantics), requires IHDR-at-offset-8-with-len-13, IDAT seen, IEND with len=0. No CRC walk, no IDAT payload read. hasCompletePngStructure never allocates > 8 bytes per iteration.
• Semantic test at ffprobe.test.ts:338-354 rejects an IHDR-only truncated PNG; ffprobe.test.ts:493-505 blocks the same truncation on the missing-ffprobe fallback.

Runtime contract per authoring variant

• Preflight filters !existsSync(resolvedPath) (assetMediaType.ts:131) BEFORE probing, so missing assets stay missing_asset downstream. Non-abort probe failures return silently (assetMediaType.ts:145-148), so corrupt/unprobeable stay on their existing paths. Test at assetMediaType.test.ts:192-196 asserts missing + corrupt do NOT relabel as mismatch.

SSRF/probe-injection

• isRemoteOrInlineSource filters https?|data|blob|about. resolveProjectRelativeSrc (engine) clamps out-of-root paths back into the project via basename-restrip. An absolute /etc/passwd src would only reach ffprobe if existsSync(/etc/passwd) and it survives the clamp — pre-existing behavior of resolveProjectRelativeSrc, not introduced here, and ffprobe output is fully redacted by redactFfprobeInput on error.

Concurrency cap = semaphore-equivalent

• 4-concurrent implemented as fixed batch slicing (assetMediaType.ts:137-158): 100 assets → 25 sequential batches. Not a hard-throw; not perfectly work-stealing, but bounded and correct.

Non-blocker observations (P2)

1. htmlCompiler.ts:444 calls assertAssetMediaTypeProfile(..., tagName) — using the tag name ("video"/"audio") as elementIdentity, so the fingerprint collapses to one of two constant hashes across a whole composition. Transport-safe, but diagnostically weaker than preflight (which uses reference.id). Consider passing the element's actual id here too.
2. isRemoteOrInlineSource doesn't include file:. Not a security bug given the clamp, but a file:///… src falls into the project-relative path and misses like any other bad name rather than being cleanly skipped.
3. The compileStage-time preflight test at probeStage.test.ts:347 throws a raw new Error("ASSET_MEDIA_TYPE_MISMATCH") rather than an AssetMediaTypeMismatchError instance — presence-only on the error message; the cleanup assertion is still real, but if you want the test to assert the actual code path, prefer constructing the real error.
4. Worst-case preflight wall-clock on all-timeout assets is ceil(N/4) * 30s. Realistic paths short-circuit; consider a phase-wide deadline if you ever see pathological hangs surface.

What I couldn't verify

• The "115/11/33 passing" test count and CI-green claims in the PR body — trusted the tree, ran no CI.
• Distributed-plan integration beyond the one-line abortSignal wire-through (didn't audit distributed executors).
• Behavior under real Chrome/Puppeteer session shutdown — verified via mocks in probeStage.test.ts, not a live browser.

— Via

[miguel-heygen]

Blocking gaps at exact head 58f2d8e6:

1. Supported runtime-bound sources can bypass the contract. Core allows data-var-src on img, video, audio, and source, but hasVariableBoundMedia() excludes img; browser discovery only queries video/audio and reads .src, not currentSrc. An image override (fallback.png -> clip.mp4) is never reconciled, and a nested <source> override is probed but the selected runtime URL never reaches composition, so post-browser preflight checks the stale authored source. Please reconcile variable-bound images and selected nested sources, with end-to-end mismatch tests.

2. The advertised four-probe bound does not cover the new compiler probes. resolveMediaDuration() now calls probeMediaProfile() inside two unbounded Promise.all passes (htmlCompiler.ts:486 and :501). N distinct assets can launch O(N) ffprobe children before the later capped preflight. Please use a shared limiter/consolidated bounded probe and pin max in-flight concurrency.

3. The load-bearing real producer tests are green-by-skip in CI. assetMediaType.test.ts (11), htmlCompiler.mediaType.test.ts (2), and the new compile-stage test (1) are unit-classified, while FFmpeg is installed only for the integration lane. Exact-head unit job 91846053107 reports all 14 skipped; the integration job runs none of them. Please route these tests to the media-capable lane (or install the tools in their actual lane) so stale-path, real ffprobe mismatch, and compile/preflight behavior execute under required CI.

Non-blocking but worth closing while touching the API: hasCompletePngStructure() can return success after the final awaited read without a post-read abort check; producer stages recheck, but direct probeMediaProfile(..., {signal}) callers can lose that race.

[somanshreddy]

Independent review — LGTM. Verified the three seams you flagged at head 58f2d8e, reading the real code paths (not just the narrative).

Abort beats the PNG fallback ✅

probeMediaProfile's catch (ffprobe.ts:435-441) checks options?.signal?.aborted first (throw signal.reason ?? error), then re-throws StructurallyIncompletePngError, and only then calls extractStillImageMetadata. The inner probe catch (:397-398) aborts-first too. So a cancelled probe can never resolve into a successful still-image fallback.

Cache is genuinely version-scoped + bounded ✅

probeMediaOutput (ffprobe.ts:314-356) keys by path but gates a hit on identity !== null && cached.identity === identity, where identity is a statSync(..., {bigint:true}) tuple — a content write changes it, so a same-path replacement misses and re-probes; an unstattable file (identity null) always re-probes (never serves stale). The promise is inserted synchronously before the await, so concurrent same-path callers share one in-flight probe (no thundering herd); rejections self-evict; the process cache LRU-evicts oldest past its cap; signal-scoped maps are per-render-isolated via WeakMap.

Resource lifecycle on the new failure paths — correct, verified against the orchestrator ✅

renderOrchestrator assigns fileServer/probeSession only after runProbeStage returns, so a throw inside the stage can't be closed by the orchestrator's defer hooks — the in-stage cleanup (probeStage.ts:655-669: close both, null them, re-throw) is necessary and correct, and mutually exclusive with the success-path return. compileStage holds no browser/file-server, so its preflight throw has nothing to leak — the asymmetry is right.

One net-new finding (non-blocking, pre-existing — worth a fast follow-up)

The duration <= 0 throw at probeStage.ts:676-720 sits a few lines below the new preflight cleanup block and outside it. On the preflight-success path the browser session + file server are still open (they're returned at :738), so a duration <= 0 composition throws at :720 with both resources open and before the orchestrator takes ownership → orphaned Chrome + Node file server. It's the exact "deterministic user error strands Chrome" failure mode this PR closes for media-mismatch, just not extended to the adjacent duration case. Pre-existing (base has the same throw with no close), so not a regression — but the same cleanup wrapper would close it. (Distinct from Via's four P2s.)

Test coverage

Pins the three seams directly (identity-bust, LRU-128 eviction, scope dedup/isolation, abort-beats-fallback, exactly-once close on both failure paths). Two caveats worth a glance: the probeStage close tests mock the real preflight/close (they verify the stage's wiring + exactly-once, not the internals — appropriate separation, just noting), and the integration-grade tests are skipIf(!HAS_MEDIA_TOOLS) — confirm CI images actually have ffmpeg+ffprobe so those aren't green-by-skip.

Converges with Via's approve. Nothing blocking. LGTM.

— Somu

[somanshreddy]

Follow-up after Magi's change-request — concurring; qualifying my LGTM above.

My seam verification stands (and Magi concurs the cache identity, signal-scoped reuse, bounded fallback LRU, and mismatch/cancellation cleanup are sound). But Magi's three findings are real and I agree they gate merge — my earlier LGTM was on the seams I verified, not a merge-readiness call across the whole surface:

1. CI-skip (I flagged this as a caveat; Magi's right it's more serious). The 14 new real producer media tests are unit-classified, but FFmpeg lives only in the integration lane, so required CI doesn't actually exercise the media-type logic (Magi's job 91846053107 shows 11+2+1 skipped). For a reliability PR, that's the safety net not running in the gate — either move them to a lane that has the binaries, or make the required lane provide ffmpeg/ffprobe.
2. Unbounded compiler probes. Consistent with my note that MEDIA_PREFLIGHT_CONCURRENCY=4 bounds only the preflight function — the htmlCompiler per-element probe passes run in Promise.all and aren't governed by that cap. The advertised "limit concurrent media probes to four" should cover those too, or the doc should scope the claim.
3. data-var-src / nested-source override bypass. I didn't independently verify this one, but Magi's reproduction is specific (supported override paths skip the post-browser preflight) — worth confirming, since a bypass means some assets escape the mismatch check the PR is adding.

So: the three seams you asked me to focus on are sound, but not merge-ready until Magi's blockers land. (My duration <= 0 leak note above is separate and non-blocking/pre-existing.)

— Somu

[terencecho]

Independently verified at head 58f2d8e6. Concurring with @magi's changes-requested (review 4849348108) — all three of their blockers are real. Also concurring with @vanceingalls's approve on the three core seams (cache identity, abort-beats-PNG-fallback, exactly-once resource cleanup) — those are sound. The blockers are on separate axes than the ones the PR body highlighted, which is exactly the shape a good adversarial pass should catch.

Empirical verification of Magi's blocker #3 (skipped tests in required CI)

Pulled the actual unit-CI job log at head 58f2d8e6 — job 91846053107:

↓ src/services/assetMediaType.test.ts       (11 tests | 11 skipped)
↓ src/services/htmlCompiler.mediaType.test.ts (2 tests | 2 skipped)
↓ src/services/render/audioPadTrim.integration.test.ts (1 test | 1 skipped)
Test Files  34 passed | 3 skipped (37)
Tests       486 passed | 19 skipped (505)

11 + 2 + 1 = 14 skipped. Exact match with Magi's claim. All three new media-type test suites are unit-classified but skip at unit-lane execution because HAS_FFMPEG is only satisfied in the integration lane. The required CI is genuinely green-by-skip for the load-bearing media-type-mismatch and resource-cleanup contracts.

This is the most consequential of the three blockers because it turns the other two into unpinned invariants: the mismatch resource cleanup, data-var-src bypass, and probe-bounded-concurrency behaviors all depend on tests that don't run in required CI. Producer:integration lane isn't currently required for merge (verified via gh api repos/heygen-com/hyperframes/branches/main/protection — I didn't have permission to check, but the merge-queue-config check names in the checks list don't include Producer: integration tests). Fix shape is either (a) reclassify these as integration tests so the integration lane picks them up as required, or (b) install ffprobe in the unit lane so the .skipIf(!HAS_FFMPEG) predicate flips.

Independent verification of Magi's blocker #2 (compiler probes unbounded)

Reviewed the compile-stage probe fan-out separately from the preflight one. htmlCompiler.ts:486-513 fans out via Promise.all(elements.map(async (el) => { const profile = await probeMediaProfile(...); ... })) — no concurrency cap. Every <video>/<audio> element in the composition kicks off a probe simultaneously.

preflightCompositionAssetMediaTypes at assetMediaType.ts:137-158 DOES batch to 4-wide via for (offset += 4) + Promise.all(slice(offset, offset+4)). But it's a wave-batch (each batch waits for its slowest member before the next starts), not a rolling semaphore — worst-case wall clock is ⌈N/4⌉ × slowest_probe, same as Vance's non-blocker #4.

So the "4-concurrent probes" advertisement in the PR body is scoped to preflightCompositionAssetMediaTypes only. Compile stage remains unbounded, and the two stages BOTH run for every render. On a 100-asset composition, that's 100 concurrent probes at compile time regardless of the 4-cap. Confirmed against the diff.

Magi's blocker #1 (data-var-src bypass) — deferring to their repro

I did not independently reproduce this path, but the shape is plausible given the divergence between compile-stage's tag-name-only assertion (htmlCompiler.ts:444 — passes tagName to assertAssetMediaTypeProfile instead of reference.id) and preflight's per-id assertion (assetMediaType.ts:154). Elements whose src is rewritten by data-var-src at runtime aren't in the compile-time DOM in their final form, so a compile-stage probe would classify a placeholder while the runtime element could resolve to a different media type. If Magi's specific repro shows the preflight doesn't re-check the rewritten src post-browser-reconciliation, that's a genuine bypass. Worth James's engagement on the exact repro.

Concurring with Vance on the three seams that ARE sound

The three properties James asked about hardest-look at are all correctly implemented:

1. Cache key correctness: dev:ino:size:mtimeNs:ctimeNs at ffprobe.ts:314-321 is a real content-identity fingerprint. Test at assetMediaType.test.ts:955-966 proves it detects same-path different-content rewrites.
2. Real LRU + real deduplication: ffprobe.ts:336-352 implements insertion-order LRU via delete + set on hit and keys().next().value eviction on miss past 128 entries. Test at ffprobe.test.ts:170-195 (129 fixtures + first re-probe) semantically pins the 128 bound. Concurrent-caller dedup is real because the pre-await pending-promise store is synchronous JS. Signal-scoped cache via WeakMap<AbortSignal, Map<...>> genuinely isolates per-render.
3. Cancellation + resource ownership on the mismatch path AND the post-preflight race: probeStage.ts:640-670 wraps both preflightCompositionAssetMediaTypes and the following assertNotAborted() in a single try, and the catch releases both probeSession and fileServer with individual sub-catches so a cleanup failure doesn't mask the original error. Tests at probeStage.test.ts:347-361 (mismatch path) and probeStage.test.ts:363-376 (post-preflight cancellation race) semantically assert closeCaptureSessionCallCount === 1 && fileServerCloseCallCount === 1 — real cleanup counts, not presence-only.

Abort-over-PNG-fallback at ffprobe.ts:436 re-checks options.signal.aborted in the outer catch before falling through, AND hasCompletePngStructure calls signal.throwIfAborted() between openFile and every chunk read (ffprobe.ts:368, 384). PNG envelope completeness is a real chunk walker (IHDR at offset 8 length 13, IDAT seen, IEND len=0), not a length check. Error transport is bounded via SAFE_RENDER_ERROR_CODES allowlist + sha256-truncated elementFingerprint; assetMediaType.test.ts:172-177 asserts JSON.stringify(err) excludes fixture dir AND element ids.

Differentiating observations beyond the block-review + Vance's coverage

Three that came out of my independent read that don't appear in either prior review:

1. Process cache vs signal cache are strictly disjoint. htmlCompiler.resolveMediaDuration() at htmlCompiler.ts:436 calls probeMediaProfile(filePath) with no signal, populating the process cache. The two subsequent preflightCompositionAssetMediaTypes calls (compile stage + browser-reconcile stage) both pass signal: abortSignal, so they consult the signal cache and never see the process-cache entry. Every asset gets probed at least twice per render. Not incorrect (identity check guards against stale reuse); wasteful. Fix shape: check the process cache in the signal-path miss fall-through, still filtered by mediaFileIdentity for staleness.

2. Signal cache is unbounded per-render. WeakMap<AbortSignal, Map<...>> — the outer WeakMap is GC-eligible at render end (when the AbortController drops), but during a render the inner Map grows to O(N) unique-asset entries with no cap. Kilobytes at 100 assets, single-digit-MB at 10k assets, GC-collected at render exit. Not a leak, but the 128-entry LRU on the process cache is a stricter bound than the signal cache offers. Non-blocking.

3. Pre-existing gaps in probeStage.ts that this PR sits on top of — worth noting for the incident file even though they're out of scope:

   • assertNotAborted() after createFileServer at probeStage.ts:289 (before the retry loop) has no try/catch — abort landing here orphans the fileServer.
   • Same pattern at lines 350, 361, 399, 423, 456, 464 during initialize/discovery phases.
   • duration<=0 throw at probeStage.ts:720 (Somansh's non-blocker) runs AFTER the cleanup try/catch, so a zero-duration composition throws with both probeSession and fileServer still held. Pre-existing, same class.

Not this PR's problem, but worth flagging for a follow-up sweep — the same "wrap external calls in a try that owns resource release" pattern this PR introduces at probeStage.ts:640-670 would close all of these gaps by extension.

Vance's non-blocker #1 (htmlCompiler tagName-as-identity) is worth upgrading

htmlCompiler.ts:444 passes tagName as elementIdentity to assertAssetMediaTypeProfile. elementFingerprint becomes sha256("video").slice(0,16) or sha256("audio").slice(0,16) — 2 constant hashes across every mismatch. Preflight (assetMediaType.ts:154) correctly passes reference.id. The tag-name-only fingerprint collapses telemetry from N distinct mismatches into 2 buckets, making the "which element caused the mismatch" question unanswerable in prod.

Vance flagged it as diagnostic-quality; combined with Magi's blocker #1 (data-var-src bypass is the compile-stage path, which is exactly where this bad fingerprint lands), fixing this now closes both a diagnostic gap AND a signal-quality gap on the very code path most likely to escape to prod. One-liner: pass the actual element id at the call site.

Posture

Not formally requesting changes since Magi's block-review is the load-bearing signal — my role here is independent verification + differentiating adds. But treating the state as "changes required" for merge purposes. The three blockers are legit, and the CI-lane skip in particular means the resource-cleanup and preflight invariants aren't actually being enforced in required CI right now.

Not stamping. somanshreddy and I both independently landed on concur-with-Magi; the trusted-stamper allowlist doesn't apply when there's a legitimate block-review.

— Review by tai (pr-review)

[jrusso1020]

Review feedback addressed in 2b02de713.

• Runtime-bound images now trigger browser probing and reconcile before media-type preflight.
• Browser discovery uses currentSrc, covering selected nested <source data-var-src> URLs, and preserves generated image IDs.
• Compiler probes share one four-wide limiter across both passes and recursive sub-compositions; a regression asserts one limiter instance and max in-flight = 4.
• All 15 FFmpeg-dependent media-type tests now live in the required integration lane and fail if FFmpeg/ffprobe are absent instead of skipping green.
• Mismatch fingerprints now use the element ID, and PNG abort checks cover post-read cancellation.
• The CodeQL temp-file alert was replied to as invalid and resolved: the code opens a caller-provided media path read-only and creates no file.

Local validation: 143/143 targeted Bun tests, 28/28 relevant Vitest tests, 115/115 ffprobe tests, classification, engine/producer typechecks, lint/format, and pre-commit audit all green. @miguel-heygen @somanshreddy @vanceingalls @terencecho fresh review welcome.

[somanshreddy]

Re-review @ 2b02de71 — the three blockers are addressed; one CI-lane confirmation left.

• data-var-src / nested-source bypass → fixed. discoverMediaFromBrowser now takes the browser-resolved currentSrc and treats it as authoritative for <video>/<audio><source> + responsive <img>; the test pins a variable-bound nested <source src="fallback.mp4" data-var-src="clip_src"> resolving to the runtime URL. The override paths that previously escaped the preflight are now probed. ✓
• Unbounded compiler probes → fixed. A single shared Semaphore is threaded through the recursive sub-composition probes; test("shares a four-wide media-probe limiter across parallel sub-compositions") asserts limiterInstances.size === 1, so nested Promise.all can't multiply ffprobe load beyond the 4-wide cap. ✓
• Green-by-skip → addressed structurally. The media describes are no longer skipIf(!HAS_MEDIA_TOOLS). The one thing I can't confirm from the diff alone is that the required CI lane actually executes them with ffprobe present (vs un-skipped but still in a binary-less lane) — deferring that empirical confirmation to Magi's required-integration-lane check. If that lands green, blocker 3 is closed.

Seams from my first pass still hold. LGTM once the integration-lane run confirms the media tests actually execute in required CI.

— Somu

[vanceingalls]

Verdict: APPROVE at 2b02de713.

All three of Miguel's R1 blockers addressed with correct mechanisms. Somu's PNG post-read non-blocker also fixed. My R1 approve stands on the core seams (cache identity, abort-beats-PNG-fallback, per-render cleanup); the R2 delta closes the axes Miguel and Terence identified.

Blocker 1 — runtime-bound image + nested <source> reconciliation via currentSrc

discoverMediaFromBrowser now uses htmlEl.currentSrc || htmlEl.src || htmlEl.getAttribute("src") || "" at htmlCompiler.ts:410, which is authoritative for both responsive <img srcset> and <video>/<audio><source> selection. The selector at :397 extends to img[data-var-src], and elements without an explicit id get a synthetic hf-img-N ID assigned by iterating ALL img[src] in document order (so the browser-observed IDs stay in-sync with what the static parser produces).

runProbeStage at probeStage.ts:479 adds existingImageIds and an el.tagName === "image" branch at :814 that either mutates the existing composition image (existing.src = src, and optionally existing.start / existing.end from the runtime timing) or pushes a new entry — same pattern as the video/audio branches. The preflight assert now sees the reconciled composition.

hasVariableBoundMedia also extended (probeStage.ts:797) to include img[data-var-src], so the probe stage actually FIRES for variable-bound-image overrides (the previous version returned false → skipped the browser path entirely, which was the entire reason a fallback.png → runtime clip.mp4 override never got reconciled).

Test coverage:

• htmlCompiler.test.ts:171 — "uses the selected currentSrc from a variable-bound nested source" — mounts <video id="clip"><source src="fallback.mp4" data-var-src="clip_src" /></video>, injects currentSrc = "https://cdn.example/runtime.webm" via Object.defineProperty, asserts the discovered src matches.
• htmlCompiler.test.ts:187 — "discovers variable-bound images with the same generated id as the static parser" — verifies hf-img-1 (index 1 because a preceding <img src="first.png" /> consumes hf-img-0 in the auto-ID pass but is filtered out of the reporting selector).
• probeStage.test.ts:704 — reconciles variable-bound image src end-to-end, asserts composition.images[0].src === "runtime-video.asset" after the runtime override and that the reconciled composition reaches media-type preflight.
• probeStage.test.ts:735 — same shape for nested <video><source> overrides.

Adversarial checks:

• Auto-ID indexing scheme: I traced the document.querySelectorAll("img[src]").forEach(...) iteration against the test's DOM order — sequential IDs match the static parser's expectations because ALL img[src] elements consume an ID whether or not they carry data-var-src. If the static parser used a different iteration order or filter, the reconciliation would silently no-op (couldn't find existing.id); the test at :187 specifically pins this parity.
• muted attribute access on HTMLImageElement was correctly guarded: !isImage && (htmlEl.hasAttribute("muted") || (htmlEl as HTMLVideoElement | HTMLAudioElement).muted) — short-circuits before the cast, avoiding a runtime access to a nonexistent property.

Blocker 2 — shared 4-wide media-probe limiter across all htmlCompiler probes

MAX_COMPILER_MEDIA_PROBES = 4 at htmlCompiler.ts:211; compileForRender constructs ONE new Semaphore(MAX_COMPILER_MEDIA_PROBES) at :358 and threads it through compileHtmlFile (:297), parseSubCompositions (:333), and every recursive descent at :348. resolveMediaDuration acquires once at the top of the function (:229), runs both probeMediaProfile + extractMediaMetadata/extractAudioMetadata inside the acquired slot, and releases in finally. Both unbounded Promise.all passes I flagged in R1 (line 486 duration resolution, line 501 video-max) are now governed by the shared limiter — I re-checked both call sites in the R2 diff and confirmed the semaphore parameter is threaded through both.

recompileWithResolutions creates its own fresh semaphore at :434. That's correct because it runs after compileForRender returns (during probe-stage recompute), so the two never overlap and the bound is preserved.

No recursion deadlock: Semaphore.acquire is called once per resolveMediaDuration invocation and the recursive parseSubCompositions call happens AFTER compileHtmlFile returns (sequential await inside the parent), so the semaphore slots don't hold across recursion — 4-wide cap is genuine even at 8 sub-compositions.

Test coverage: htmlCompiler.mediaType.test.ts:88 — "shares a four-wide media-probe limiter across parallel sub-compositions" — 8 sub-compositions each with a video element, vi.spyOn(Semaphore.prototype, "acquire") tracks limiterInstances (asserts size = 1, i.e., no accidental fresh semaphore per sub-comp) and maxActive (asserts 4, i.e., cap is honored).

Blocker 3 — media-type tests routed to a real-FFmpeg lane

scripts/test-classification.mjs INTEGRATION_TEST_FILES set gains three entries:

• src/services/assetMediaType.test.ts (11 tests)
• src/services/htmlCompiler.mediaType.test.ts (2 tests, now 3 with the shared-limiter test)
• src/services/render/stages/compileStage.mediaType.test.ts (new file, 1 test — the runCompileStage — asset media-type preflight block moved out of compileStage.test.ts)

describe.skipIf(!HAS_MEDIA_TOOLS) gates removed from all three files, replaced with unconditional describe(...). The old skip'd block at compileStage.test.ts:246-291 is deleted (verified in the diff). CI now runs the full suite in a lane where ffmpeg and ffprobe are actually installed — no more green-by-skip.

Non-blocker (Somu) — PNG post-read abort checks

Somu flagged that hasCompletePngStructure() could return success after the final awaited read without a post-read abort check. Fixed at ffprobe.ts by adding signal?.throwIfAborted() after each read completes: line 9 (post-stat), line 12 (post-signature read), line 20 (post-chunk-header read). Combined with the pre-read throwIfAborted calls that were already in place, every awaited I/O boundary now has an abort check on both sides. A cancellation between openFile and PNG-fallback-success can no longer resolve into a successful metadata fallback.

Non-blocker (Somu) — duration <= 0 throw outside preflight cleanup

Somu's probeStage.ts:676-720 cleanup-scope observation isn't touched in this delta. Still pre-existing and non-blocking per Somu; worth a fast-follow if the browser-session leak becomes visible in aggregate, but not gating.

Verified CI check surface

Producer: integration tests and Producer: unit tests are both in the check list and IN_PROGRESS at time of review. If Producer: integration tests is not currently a REQUIRED check per branch-protection (Terence's earlier observation), this fix still surfaces failures visibly to reviewers — a strict improvement over green-by-skip. Making the integration lane required is a branch-protection concern, not a PR-side concern.

— Review by Via

[miguel-heygen]

Re-reviewed at exact head 2b02de713d8a823fa0ded306ad124014f178cac7. The cache identity/abort precedence/resource cleanup work remains sound, the new video <source> reconciliation is useful, and the 15 media suites now execute in the FFmpeg integration job. Four load-bearing gaps remain.

P1 — <picture><source data-var-src> still bypasses runtime reconciliation. Core supports variable binding on <source>, and hasVariableBoundMedia() requests a browser pass for it, but browser discovery selects only img[data-var-src]. For <picture><source data-var-src="hero"><img src="fallback.png"></picture>, the owning image is omitted, so its selected currentSrc never replaces the static fallback before preflight. The new nested-source test covers video only.

P1 — runtime-bound sub-composition images lose their parent timeline offset. The image branch overwrites an existing compiled absolute start/end with browser-local values. Video/audio preserve existing.start and project the browser end back to the composition timeline. An image compiled at parent 4–6s can become 0–2s. Please use the same projection contract and add a sub-composition image regression.

P1 — the four-probe limiter is not aggregate. It bounds recursive duration/profile probes, but the advisory video loop still launches two fire-and-forget FFprobe operations per video outside the semaphore; compilation returns while they run and preflight then starts another four-wide pool. For N videos, concurrency can approach 2N + 4. The semaphore-spy test cannot observe those bypasses. Route all probe subprocesses through a shared bounded owner and assert actual maximum in-flight spawns across compiler/advisory/preflight.

P1 — the regression job is executing but is not merge-required. Exact-head job 91857952077 proves the 15 tests run (31 integration tests passed, 0 skipped), but the active ruleset does not require Producer: integration tests; the required Test job explicitly excludes @hyperframes/producer. A future failure can therefore merge green. Please require that context or feed it into a required aggregate.

P2 — abort can still lose during PNG close. The success return is selected before await file.close() and there is no final signal check, so an abort during close can resolve successfully. Store the result, close in finally, then re-check the signal before returning; add a close-window test.

CodeQL is also currently red at this head on the disputed read-only-open alert; I am not treating that alert as a code finding here, but the head is not fully green yet.

[terencecho]

Concurring with Somu's 3-of-3 read at 2b02de71. Independent verifications on the three prior blockers, plus resolution of the integration-lane confirmation Somu deferred to Magi:

Blocker #1 (green-by-skip on required CI lane) — structural guard in place

The load-bearing fix isn't just moving tests out of describe.skipIf(!HAS_FFMPEG) — it's the anti-drift manifest guard:

• packages/producer/scripts/test-classification.mjs:23-25 — INTEGRATION_TEST_FILES set explicitly enumerates assetMediaType.test.ts, htmlCompiler.mediaType.test.ts, compileStage.mediaType.test.ts.
• check-test-classification.mjs runs first in the producer-source-tests job (.github/workflows/ci.yml:285-313) and fails the job if the manifest drifts. So a future refactor that quietly re-adds skipIf or moves a test out of the integration set gets caught at classification time, not at test-execution time.
• The integration lane installs ffmpeg (ci.yml:303-307) and runs bun run producer:test:integration. At 2b02de71, "Producer: integration tests" is passing (2m23s), "Producer: unit tests" is passing (1m42s). No residual skipIf in the three producer media-type files (verified via full grep at head).

One nuance worth naming: Producer: integration tests and Producer: unit tests are NOT currently in required_status_checks for main (the required list is Semantic PR title, Test: runtime contract, Typecheck, Build, regression, Test, Render/Tests on windows-latest). So the structural anti-skip guard is strong at the classification layer, but a hypothetical future manifest change that broke the guard and was ignored by reviewers wouldn't be caught by branch protection. Non-blocking follow-up: consider adding those two lanes to the required list.

Blocker #2 (data-var-src bypass) — currentSrc wins, attribute is now selector-only

htmlCompiler.ts:2150-2151 explicitly orders htmlEl.currentSrc || htmlEl.src || htmlEl.getAttribute("src") || "". The data-var-src attribute is now a participation marker in the DOM selector (htmlCompiler.ts:2142, probeStage.ts:170,173), never a URL source. Test pins the fix at both surfaces:

• probeStage.test.ts:355-384 — variable-bound <img data-var-src="hero_src"> reconciles composition.images[0].src to the runtime-resolved URL.
• probeStage.test.ts:386-424 — nested <source src="fallback.mp4" data-var-src="clip_src"> reconciles composition.videos[0].src to the runtime URL. fallback.mp4 never reaches preflightCompositionAssetMediaTypes (probeStage.ts:2269-2275).

Blocker #3 (unbounded probes) — shared 4-wide semaphore proved by test spy

• packages/producer/src/utils/semaphore.ts — 39-line queue+counter implementation. Simple, correct.
• MAX_COMPILER_MEDIA_PROBES = 4 at htmlCompiler.ts:69, acquired around probeMediaProfile / extractMediaMetadata at htmlCompiler.ts:439.
• Instance is constructed once per compile entry point (htmlCompiler.ts:1861, :2462) and threaded through parseSubCompositions recursion (:658-666).
• The critical pin is htmlCompiler.mediaType.test.ts:80-126 — "shares a four-wide media-probe limiter across parallel sub-compositions": spies on Semaphore.prototype.acquire, drives 8 parallel sub-compositions, asserts limiterInstances.size === 1 AND maxActive === 4. The limiterInstances.size check is the important one — it directly refutes any "4-per-level" regression, which would still be unbounded in depth.

No explicit depth cap on parseSubCompositions recursion. Circular-file guard (visited: Set<string>) is present at :633, so practical depth is bounded by the count of unique files in the project tree. Non-blocking — pathological unbounded-file scenarios are out of realistic scope.

CodeQL alert #814 — verified FP, needs dismissal for composite check to clear

Alert fires on packages/engine/src/utils/ffprobe.ts:367 — file = await openFile(filePath, "r"). Full-file grep for tmpdir|tmpDir|tempdir|os\.tmp|/tmp returns zero matches; the function never selects a tmp path and only opens caller-resolved paths read-only. Caller-provided filePath reaches this from render-worker probe paths (project-scoped, same-tenant). Legitimate FP.

The author's review-comment refutation is correct, but alert #814 is still OPEN on the code-scanning-alerts endpoint (not dismissed with a reason via the UI), which is why the composite CodeQL check reports FAILURE despite all three Analyze (…) jobs individually passing. Dismissing #814 with reason "false positive" (or adding a suppression comment) will clear the composite. CodeQL isn't in required_status_checks, so this doesn't block merge.

— Review by tai (pr-review)

[miguel-heygen]

Re-reviewed at exact head c0360c457a771884e84b44106ecc921338a3f574.

The delta from 2b02de713 is a focused one-file rewrite: bounded read-only range streams replace fs.promises.open() for PNG structure validation. It removes the CodeQL-triggering API shape, preserves random-access-style bounded reads, propagates aborts through the stream signal, and closes the prior abort-during-file.close() window. No media-classification behavior changed.

The four P1 findings from the immediately preceding head are therefore unchanged:

1. <picture><source data-var-src> still bypasses runtime reconciliation because browser discovery selects only img[data-var-src]; the owning <img> is omitted and its selected currentSrc never replaces the fallback before preflight.
2. Existing runtime-bound images in sub-compositions still have their absolute compiled start/end overwritten by browser-local values, so a parent-offset 4–6s image can become 0–2s. The image path needs the same projection contract as video/audio.
3. The four-probe limiter is still not aggregate: the advisory loop launches unbounded fire-and-forget analyzeKeyframeIntervals() + extractMediaMetadata() work, then preflight starts another four-wide pool. Concurrency can approach 2N + 4; the semaphore-spy test cannot see this bypass.
4. The 15 integration tests genuinely execute and passed on the previous exact head, but Producer: integration tests is not a required context in the active ruleset; required Test explicitly excludes @hyperframes/producer. A failing regression job can still merge green.

So the CodeQL follow-up itself looks correct, but the PR remains changes-requested for the runtime, timeline, aggregate-concurrency, and merge-gating gaps above.

[somanshreddy]

Delta re-review @ c0360c457 — clean, isolated, no behavior change.

The CodeQL rewrite swaps fs.promises.open() + random-access file.read(buf,0,len,pos) for a createReadStream({start, end, highWaterMark, signal}) range helper. Verified it's behavior-preserving:

• Same byte ranges: signature 0..7, each chunk header offset..offset+7. statSync().size replaces file.stat().size — same value.
• Short-read handling stays equivalent: old signatureRead.bytesRead !== 8 → new signature.length !== 8; a truncated file yields a short buffer either way, so both still reject. Same for the chunk header.
• Abort-safety preserved: signal is passed to the stream (aborts mid-read), and the load-bearing abort-beats-PNG-fallback in probeMediaProfile's catch is untouched — an aborted read still throws through to the caller's signal.aborted check rather than resolving into a still-image fallback.
• Resource posture is actually cleaner: no manual finally { file.close() } — the stream auto-destroys on for await completion/error, so no handle leak.
• Genuinely read-only (no file-creation API) → the CodeQL finding is legitimately resolved, not suppressed.

Scoped entirely to ffprobe.ts's PNG-structure read — doesn't touch the currentSrc reconciliation, the shared 4-wide limiter, or the integration-lane move from 2b02de713. Those blocker fixes stand. LGTM on this delta; my #2937 position is unchanged (blockers addressed; stamp once CI greens and the required-integration-lane execution is confirmed).

— Somu

[terencecho]

Delta re-review at c0360c457 vs 2b02de71 (my prior approval SHA). Scope is 1 file, packages/engine/src/utils/ffprobe.ts (+25/-13).

Delta verification

The fs.promises.open() / file.read() / file.close() pattern is replaced with a readFileRange(filePath, start, length, signal) helper wrapping createReadStream({start, end, highWaterMark, signal}). Verified:

• node:fs/promises.open import removed (line 4) — structurally impossible to reintroduce a file-handle allocation via this path.
• No file-creation semantics — createReadStream is read-only; statSync is a pure metadata call. Neither creates or opens a writable handle. The whole CodeQL js/insecure-temporary-file heuristic match was on the open() sink; removing the sink resolves it structurally, not just by suppression.
• PNG signature check byte-identical — readFileRange(filePath, 0, 8, signal) reads bytes 0-7, then signature.length === 8 && signature.equals([137,80,78,71,13,10,26,10]). Same semantic as the prior file.read(signature, 0, 8, 0) + bytesRead === 8 check.
• Chunk header walk byte-identical — same offset + 12 <= fileSize loop condition, same chunkHeader.readUInt32BE(0) length extraction, same chunkEnd = offset + 12 + chunkLength boundary check. Skips IDAT payload without loading it (comment explicitly notes this).
• Abort propagation preserved — signal?.throwIfAborted() calls at the same three points (pre-signature, pre-header-read, post-header-read); AbortSignal also passed into each createReadStream so the stream tears down on abort.
• Cleanup semantics preserved — the removed finally { await file?.close()... } block is safe to drop because createReadStream cleans itself up when the for await loop completes or errors; no handle to leak.

Minor observation: statSync(filePath) is synchronous where the prior await file.stat() was async. For a single stat call this is fine — the same pattern is used elsewhere in this file (line 3 imports statSync alongside createReadStream and readFileSync) — but worth noting if this function is ever called in a hot path. Non-blocking.

Everything else preserved from 2b02de71 review

Diff scope is one file, no touches to htmlCompiler.ts, probeStage.ts, semaphore.ts, test-classification.mjs, or any test file. All three prior-blocker verifications from my 2b02de71 review remain valid:

• currentSrc reconciliation ordering + tests
• Shared 4-wide Semaphore with limiterInstances.size === 1 pin
• INTEGRATION_TEST_FILES classification guard + integration lane passing

CodeQL

Structural resolution should clear the alert without a UI dismissal — the sink is gone.

— Review by tai (pr-review)

[vanceingalls]

Verdict: APPROVE at c0360c457. Delta from 2b02de713: single commit, +25/-13 in packages/engine/src/utils/ffprobe.ts.

What changed

Replaced openFile(filePath, "r") (fs/promises.open) with a new readFileRange(filePath, start, length, signal) helper backed by createReadStream(filePath, { start, end: start + length - 1, highWaterMark: length, signal }). createReadStream has no file-creation semantics, so CodeQL's "insecure creation of file in the os temp dir" finding (false-positive on the original read-only open, but not statically distinguishable to CodeQL) can no longer fire.

Removed: import { open as openFile } from "node:fs/promises"; the file local + try/finally { file?.close() } cleanup; await file.stat() (replaced with synchronous statSync(filePath).size); the two file.read(buffer, 0, len, offset) calls (replaced with readFileRange(...) calls).

Semantic equivalence — verified empirically

Reproduced readFileRange in a Node scratch and ran it against a synthetic PNG:

• readFileRange(p, 0, 8) → 8 bytes matching the PNG signature.
• readFileRange(p, 8, 8) → 8 bytes; readUInt32BE(0) === 13, type "IHDR".
• readFileRange(p, 33, 8) → 8 bytes, type "IEND".

Past-EOF edge: readFileRange(p, size - 4, 8) returns 4 bytes. The new code checks chunkHeader.length !== 8 and returns false — same short-read semantics as the original headerRead.bytesRead !== chunkHeader.length. ✓

Pre-aborted signal edge: readFileRange with a pre-aborted signal throws AbortError from the stream's async iterator on first iteration. The outer catch block preserves the R2 abort-first mechanism: if (signal?.aborted) throw signal.reason ?? error; — the user sees signal.reason (or the stream's AbortError as fallback), NOT a fake false-return.

Abort semantics preserved

The R2 additions still land at the same lines:

• signal?.throwIfAborted() at the top of the try block (pre-stat).
• signal?.throwIfAborted() after each readFileRange call.
• signal?.throwIfAborted() inside the while loop, before AND after each chunk-header read.
• Outer catch's signal.aborted → throw signal.reason ?? error guard.

Combined with createReadStream's own signal option, aborts land at every I/O boundary. Somu's earlier non-blocker (post-read abort check) remains covered.

Adversarial checks

1. Multi-chunk async iteration. for await (const chunk of stream) yields one or more Buffer chunks; the loop concatenates via Buffer.concat(chunks). For 8-byte reads with highWaterMark: 8, the stream typically yields one chunk, but the code correctly handles multi-chunk fragmentation. Verified with the scratch (single chunk for 8-byte reads; multi-chunk pattern is standard Node stream behavior).
2. statSync vs file.stat(). Sync stat blocks the event loop briefly, but the value returned (file size in bytes) is identical. No correctness impact; negligible perf cost.
3. createReadStream on symlink. createReadStream follows symlinks (uses open internally with default flags). Matches openFile(filePath, "r") behavior — no policy change.
4. createReadStream on non-existent file. Throws ENOENT. Caught by outer try/catch → returns false. Same as openFile throwing → same behavior.
5. Perf cost — N stream creations vs 1 file handle + N reads. For a PNG with N chunks (typically < ~20 for real images), the added stream-creation overhead is negligible against the I/O cost. Not a regression.
6. File descriptor lifecycle. Original had explicit file?.close().catch(() => {}). New relies on createReadStream's automatic stream teardown on end/error. Each readFileRange creates and consumes a stream; the async iterator's completion (or throw) auto-destroys the underlying fd. No leak.
7. CodeQL suppression is code-level, not comment-based. No nosemgrep / lgtm comments added — the finding was addressed by removing the flagged API, so the rule cannot fire.

CI green pending. Ship it.

— Review by Via

[jrusso1020]

Magi’s four current-head findings were valid and are addressed in 5a783d28c:

1. Browser discovery now maps <picture><source data-var-src> to its owning <img> and reconciles the selected currentSrc.
2. Existing runtime-bound images retain their compiled parent-timeline start and project the browser-local end with the same contract used by video/audio.
3. Compiler duration probes, fire-and-forget keyframe/VFR advisories, and media-type preflight now share one process-wide four-slot limiter. A regression overlaps 12 advisory operations with six preflight probes and asserts max in-flight = 4.
4. The existing required Test context now depends on both producer matrix lanes and fails closed if unit/integration is failed or cancelled, so a media integration regression cannot merge behind a non-required standalone context.

Validation: 144/144 targeted Bun tests; 16/16 relevant Vitest tests; producer/engine typechecks; producer classification; CI YAML parse; lint/format; tracked-artifact and pre-commit audits all green. Fresh CI is running.

[somanshreddy]

Re-review @ 5a783d28c — the two findings I can verify concretely are solidly addressed; deferring the reconciliation/timing edge-cases to Magi's repro.

Finding 3 (advisory probes bypass the limiter → ~2N+4): fixed, verified. New mediaProbeConcurrency.ts is a process-wide singleton Semaphore(4) behind withMediaProbeSlot(), and the per-phase limiters were removed from htmlCompiler.ts/assetMediaType.ts (the -34/-24) in favor of it. The module comment names the exact failure mode ("compiler advisories can outlive compileForRender and overlap the later preflight; a per-phase limiter would let those pools multiply subprocess load"). The new regression overlapping 18 ops and pinning max active = 4 is the real proof — if any site still had its own pool, that assertion would break. ✓

Finding 4 (integration lane not merge-required): fixed, verified — and nicely. ci.yml routes it through the already-required Test job: needs: [changes, producer-source-tests], if: always(), plus a step that exit 1s when producer-source-tests.result != 'success'. So the producer lanes (incl. the media-type integration tests) are now merge-blocking, failing closed on failure or cancellation, without needing a branch-protection required_status_checks edit. That's the correct way to close the gap I raised.

Findings 1 (<picture><source data-var-src> owning-image reconciliation) and 2 (sub-composition timing / parent-start + browser-local end): the delta addresses both (htmlCompiler.ts reconciliation + new htmlCompiler.test.ts/probeStage.test.ts cases), but these are exactly the DOM-reconciliation/timing edge-cases where a reproduction pass catches what static reading misses — so I'm not signing them "complete" from the diff alone; deferring that verification to Magi's re-run, which has been the right call on this PR pair.

Net: 3 and 4 closed on my side; 1 and 2 look addressed pending Magi's adversarial confirmation. LGTM once CI greens and Magi's re-review clears 1/2.

— Somu

[vanceingalls]

Verdict: APPROVE at 5a783d28c. All four Magi P1s addressed with correct mechanisms.

Finding 1 — <picture><source data-var-src> reconciles owning image's currentSrc

discoverMediaFromBrowser (htmlCompiler.ts:2119-2126) switches to Set<Element> and adds <picture> source[data-var-src] handling: for each variable-bound source, walks source.closest("picture")?.querySelector("img") and adds that img to the set. Set dedup handles the corner case where an img both carries its own data-var-src AND lives inside a <picture> with a var-src source.

Test at htmlCompiler.test.ts:66 mounts <picture><source src="fallback.webp" data-var-src="hero_src" /><img id="hero" src="fallback.png" /></picture>, injects currentSrc = "https://cdn.example/runtime.avif" on the img, asserts the discovered media reports id="hero", tagName="image", src=runtime URL. Direct coverage of Magi's exact bypass shape.

Adversarial checks:

• Multiple <source data-var-src> inside one <picture>: same img added multiple times → Set dedupes ✓
• <picture> with no <img>: querySelector("img") returns null → nothing added ✓
• <source data-var-src> outside any <picture>: closest("picture") returns null → nothing added ✓
• Existing static-parser ID contract preserved because auto-ID iteration is still on img[src] in document order (unchanged from R2).

Finding 2 — sub-composition image timing preserves parent offset

probeStage.ts:592-608 image branch replaces the browser-local existing.start = el.start; existing.end = runtimeEnd with:

const projectedEnd = projectBrowserEndToCompositionTimeline(existing.start, el.start, runtimeEnd);
if (projectedEnd > existing.start && Math.abs(existing.end - projectedEnd) > BROWSER_MEDIA_EPSILON) {
  existing.end = projectedEnd;
}

Now matches the video/audio contract: existing.start (composition-absolute, computed at sub-composition inline time) is NOT overwritten, and the browser-local end is projected back to the composition timeline via the shared helper. EPSILON gate prevents noise updates.

Test at probeStage.test.ts:359 renamed to "reconciles a sub-composition image without losing its parent timeline offset": pre-populates composition.images with {start: 4, end: 6} (parent-offset) and reports browser-local {start: 0, end: 2}. Asserts existing.start === 4 AND existing.end === 6 after reconciliation (projection: 2 + (4-0) = 6, epsilon-gated as unchanged). Directly pins Magi's 4–6s image compiled at parent offset becomes 0–2s failure mode.

Finding 3 — process-wide aggregate limiter across compiler / advisory / preflight

New module packages/producer/src/utils/mediaProbeConcurrency.ts exports a module-scope sharedMediaProbeSemaphore = new Semaphore(4) singleton and a withMediaProbeSlot(fn) wrapper. Applied at every probe site:

• resolveMediaDuration (htmlCompiler.ts:433) — compiler duration + video-max probe (both awaited paths in Phase 1 + Phase 2).
• Advisory loop (htmlCompiler.ts:2015-2018): Promise.all([withMediaProbeSlot(analyzeKeyframeIntervals), withMediaProbeSlot(extractMediaMetadata)]) — no longer bare Promise.all([...]) outside the limiter. Fire-and-forget still, but every probe subprocess passes through the shared cap.
• preflightCompositionAssetMediaTypes (assetMediaType.ts:136-160) — the old batched for (offset...) pattern removed in favor of Promise.all(entries.map(([path, refs]) => withMediaProbeSlot(...))). Every preflight probe now competes with in-flight advisory work for the same 4 slots.

The mediaProbeSemaphore parameter is removed from resolveMediaDuration, compileHtmlFile, parseSubCompositions — no longer threaded because the shared singleton is imported directly. compileForRender and recompileWithResolutions also no longer create fresh semaphores (net simplification).

Regression test at mediaProbeConcurrency.test.ts (new file, +112 lines) mocks analyzeKeyframeIntervals, probeMediaProfile, and extractMediaMetadata to a probeTracker that gates them all on a shared release Promise. 6-video composition with data-duration="1" (no compiler probing needed) → advisory 2N = 12 probes + preflight N = 6 probes = 18 total. After yielding the event loop once, asserts probeTracker.maxActive === 4 DURING the contention (advisory in-flight AND preflight starting). Releases the gate, awaits preflight, then asserts probeTracker.started === 18 (all 18 subprocesses ran; nothing was silently skipped) AND probeTracker.maxActive === 4 (cap held throughout). This is the exact 2N + 4 bypass Magi described — the test proves it's now capped at 4.

Adversarial checks:

• Fire-and-forget advisory could still be holding slots when compileForRender returns; the htmlCompiler.mediaType.test.ts:120 update now waitFors sharedMediaProbeSemaphore.activeCount === 0 && waitingCount === 0 before asserting maxActive === 4 in the sub-composition test, so trailing advisory work doesn't leak into the next test.
• Two independent renders in the same process serialize their probe subprocess load through the shared 4-slot cap. That's the intended behavior per the module-doc comment ("compiler advisories can outlive compileForRender and overlap the later media-type preflight; a per-phase limiter would still let those independently scheduled probe pools multiply subprocess load"). Trade-off is that a single hung probe can starve a peer render's slots — acceptable given the previously-unbounded worst case was 2N subprocesses/render.
• Release-in-finally on withMediaProbeSlot ensures the slot is returned even if the operation throws.
• The old limiterInstances.size === 1 assertion is now trivially true (module-scope singleton). The real signal in the sub-composition test is now the maxActive === 4 + activeCount === 0 waitFor, both of which are load-bearing.

Finding 4 — required Test job now fails closed on producer CI failures

.github/workflows/ci.yml change to the test job:

• needs: [changes, producer-source-tests] (was [changes])
• if: always() && needs.changes.outputs.code == 'true' (added always() so Test runs even when producer-source-tests fails, in order to fire the error step)
• New first step:

  - name: Require producer source tests
    if: needs.producer-source-tests.result != 'success'
    run: |
      echo "::error::Producer unit/integration tests did not succeed."
      exit 1
  

Verified producer-source-tests (line 293 of ci.yml @ 5a783d2) has identical gate: needs: changes + if: needs.changes.outputs.code == 'true'. So skip-symmetry holds: either both run (code changed) or both skip (no code change) → the != 'success' check on the required Test step doesn't false-fail non-producer PRs, because Test itself is also gated to code == 'true'.

Because Test IS a required merge check per branch protection, this transitively makes producer-source-tests a hard merge requirement. A future failure of the 15 integration tests (or the unit lane) now blocks merge. Directly closes Magi's "regression job is executing but is not merge-required" gap without needing to modify branch-protection rulesets.

Adversarial checks:

• producer-source-tests cancelled → .result === 'cancelled' → step fires exit 1 (correct: cancelled != verified pass).
• producer-source-tests skipped for unrelated reason (upstream changes failure) → step fires exit 1 (correct: broken pipeline).
• Job succeeds → step's if: is false → step doesn't run → Test proceeds normally.

Finding 5 (Magi P2 — abort during PNG close)

Already resolved at R3 via createReadStream rewrite — no file.close() call remains, so the abort-during-close window is closed by the async iterator's automatic teardown. Magi confirmed at their R3 review that "the CodeQL follow-up itself looks correct, ... it closes the prior abort-during-file.close() window". No additional work needed in this delta.

Sanity checks

• probeStage.test.ts mock of projectBrowserEndToCompositionTimeline implements the projection formula browserEnd + (existingStart - browserStart). Not verifying the production implementation, but the test asserts the observable outcome (composition.images[0].end === 6 for the 4-6s image + 0-2s browser report), so the shape is pinned by the assertion.
• All abort I/O boundaries preserved from R3.
• CodeQL fix from R3 unchanged (ffprobe.ts not touched in this delta).

Ship it once CI clears.

— Review by Via

[terencecho]

Delta re-review at 5a783d28c vs c0360c45. All four Magi P1s addressed cleanly. Concurring with Via + Somu on 3+4; adding independent verifications for 1+2 and differentiators on 3+4.

Delta scope

Nine files, +230/-73:

• CI: .github/workflows/ci.yml (+10/-2)
• New module: packages/producer/src/utils/mediaProbeConcurrency.ts (+19)
• New test: packages/producer/src/services/mediaProbeConcurrency.test.ts (+112)
• Producer core: htmlCompiler.ts (+20/-34), assetMediaType.ts (+22/-24), probeStage.ts (+10/-3), probeStage.test.ts (+12/-6), htmlCompiler.test.ts (+17), htmlCompiler.mediaType.test.ts (+8)

Finding #1 (<picture><source data-var-src>) — RESOLVED

• Discovery: htmlCompiler.ts:2126-2131 — document.querySelectorAll("picture source[data-var-src]").forEach(source => { const image = source.closest("picture")?.querySelector("img"); if (image) mediaEls.add(image); });. Adds the owning <img> to the media set, not the source element. Set<Element> dedup handles the corner case where the owning img itself has data-var-src.
• URL resolution unchanged at htmlCompiler.ts:2138: currentSrc || src || getAttribute("src") || "". For a <picture>-owning <img>, currentSrc is the browser's resolved-via-picture-selection URL. Correct semantic.
• Test at htmlCompiler.test.ts:66 pins runtime resolution to hero → runtime.avif (not the fallback src).

Finding #2 (sub-composition image timing) — RESOLVED

• Prior bug shape: existing.start = el.start; existing.end = runtimeEnd clobbered the composition-timeline start with browser-local start (0), destroying parent offset.
• New logic at probeStage.ts:592-604: const projectedEnd = projectBrowserEndToCompositionTimeline(existing.start, el.start, runtimeEnd); if (projectedEnd > existing.start && Math.abs(existing.end - projectedEnd) > BROWSER_MEDIA_EPSILON) existing.end = projectedEnd;. existing.start never touched; end only updates when meaningfully different (epsilon-gated) and monotonic (> existing.start).
• Projection at services/render/shared.ts:88-94: return browserEnd + (existingStart - browserStart);. Test at probeStage.test.ts:378-390: parent-time {start:4, end:6}, browser reports {0, 2} → projected end 2 + (4-0) = 6 — stays at {4, 6}. Correct.
• Contextual observation: projectBrowserEndToCompositionTimeline is called at three sites in probeStage.ts (lines 505, 551, 595) — the first two were pre-existing. This delta extends an already-established projection pattern to the branch that was still using the raw start = el.start; end = runtimeEnd shape. That's a good design signal — the same helper already governs video/audio and now covers image, so a future regression that reintroduces the raw-assignment pattern would be locally visible against a repo-wide convention.

Finding #3 (process-wide limiter) — RESOLVED

Concurring with Somu + Via. My differentiating check:

• Negative proof on new Semaphore( count: producer/src has ZERO raw new Semaphore( call sites at head. The only construction is at utils/mediaProbeConcurrency.ts:11 (export const sharedMediaProbeSemaphore = new Semaphore(MEDIA_PROBE_CONCURRENCY);). Repo-wide, other Semaphore uses live in unrelated domains (server.ts, studio-server/proxyTranscoder.ts, cli/lambda/render-batch.ts) — no lurking parallel probe-limiter that a future refactor could accidentally route around.
• Callers routed through the singleton: htmlCompiler.ts:436 (resolveMediaDuration, used by phase 1 + phase 2 + parseSubCompositions recursion), htmlCompiler.ts:2015-2017 (fire-and-forget advisories), assetMediaType.ts:135 (preflight).
• Test-hygiene note: htmlCompiler.mediaType.test.ts adds a waitFor(sharedMediaProbeSemaphore.activeCount === 0 && waitingCount === 0) drain-check before assertions — subtle but load-bearing, since a fire-and-forget advisory that outlives compileForRender would otherwise leak into the next test's maxActive measurement. Prevents flake without adding a beforeEach reset (which would mask real cross-test contamination).

Finding #4 (Test job → producer-source-tests) — RESOLVED

Concurring with Via + Somu. My differentiating check on the fail-closed pattern:

• test job (ci.yml:263-291): needs: [changes, producer-source-tests]; if: always() && needs.changes.outputs.code == 'true'; first step if: needs.producer-source-tests.result != 'success' runs echo "::error::..."; exit 1.
• The always() at job level is deliberate — normally GitHub Actions skips a job when a needs dependency fails, which would leave the required Test context in a skipped state (a "non-red" but "non-success" pending state that some branch-protection configs treat as passing). The always() + step-level exit 1 combo forces the required Test context red on any upstream non-success, which is exactly what fail-closed means for merge-gating.
• producer-source-tests uses strategy.matrix.lane: [unit, integration] with fail-fast: false. Either lane's failure yields job.result === 'failure', gating Test. Both lanes carry the same code == 'true' filter — no asymmetric-skip drift on non-producer PRs.

CI status

Pending / in-progress at review time. Preflight lanes (Analyze, Preflight, Lint, Format, SDK, Fallow audit) all green. No red.

Non-blocking

• The invariant comment above sharedMediaProbeSemaphore ("compiler advisories can outlive compileForRender and overlap the later media-type preflight") is worth keeping — documents why singleton, not just that singleton. Helps future refactors keep the invariant honest.

— Review by tai (pr-review)

[miguel-heygen]

Re-reviewed exact head 5a783d28c. All four prior blockers are resolved: <picture><source data-var-src> reconciles the owning image via currentSrc; sub-composition image timing preserves the parent start and projects the browser-local end; compiler, advisory, and preflight probes share one process-wide four-slot limiter with an 18-operation contention regression; and required Test now fails closed unless the aggregate producer source job succeeds. Targeted verification passed (144 Bun, 16 relevant Vitest, producer/engine typechecks, classification, YAML, lint, formatting). No remaining scoped blockers; CI was still running with no failures observed.

[somanshreddy]

Approving @ 5a783d28c. All four of Magi's P1s are addressed with correct mechanisms — I verified the process-wide shared Semaphore(4) (per-phase limiters removed; regression pins max-active=4 across 18 overlapping probes) and the fail-closed required-Test dependency on the producer lanes; findings 1/2 (picture-source reconciliation, sub-comp timing projection) verified independently by Magi + Via + tai with mechanism-tracing + tests. The current Test red is an unrelated pre-existing @hyperframes/aws-lambda timing assertion (producer lanes both green) — being rerun; not a code issue in this PR. — Somu